Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(http): prevent JSON payloads from spoofing UploadedFile (original) #459

Closed
wants to merge 1 commit into from

Conversation

fergusean
Copy link
Contributor

Summary of changes

An API consumer is currently able to manually specify a JSON payload matching the shape of UploadedFile and Deepkit will accept it. This opens the application up to exploitation by bad actors providing payloads with sensitive paths.

While the developer could verify against the uploadedFiles property of HttpRequest to prevent against this manually, I believe this should be something the framework protects against, plus has the benefit of cleaner code in controllers.

Relinquishment of Rights

Please mark following checkbox to confirm that you relinquish all rights of your changes:

  • I waive and relinquish all rights regarding this changes (including code, text, and images) to Deepkit UG (limited), Germany. This changes (including code, text, and images) are under MIT license without name attribution, copyright notice, and permission notice requirement.

@codecov-commenter
Copy link

codecov-commenter commented Jul 2, 2023

Codecov Report

Merging #459 (cd105cc) into master (3fd48ea) will increase coverage by 0.13%.
The diff coverage is 100.00%.

❗ Current head cd105cc differs from pull request most recent head f9e63d9. Consider uploading reports for the commit f9e63d9 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #459      +/-   ##
==========================================
+ Coverage   78.37%   78.51%   +0.13%     
==========================================
  Files         173      173              
  Lines       18538    18550      +12     
  Branches     4843     4849       +6     
==========================================
+ Hits        14529    14564      +35     
+ Misses       4009     3986      -23     
Impacted Files Coverage Δ
packages/http/src/request-parser.ts 97.35% <100.00%> (+1.30%) ⬆️

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@fergusean
Copy link
Contributor Author

Actually it looks like this implementation is insufficient to cover more complex body types, like type unions. The test cases should be accurate for a better solution, though.

I originally tried implementing this by adding a type validator for the UploadedFile class combined with attaching a symbol in the form parser. The idea was that a symbol couldn't possibly come from JSON, so that would serve as a guarantee it came from the form parser code -- but the type serialization for a class prop with a symbol as a type converts ANY value to the symbol, so by the time the validator gets the object, what was an invalid value is now the symbol:

const UploadedFileValidator = Symbol('UploadedFileValidator');
class UploadedFile {
  validator!: UploadedFileValidator;
  // ...
}

deserialize<UploadedFile>({ validator: 'test' })
// result: { validator: UploadedFileValidator } 

deserialize<UploadedFile>({})
// result: {} 

@fergusean fergusean closed this Jul 2, 2023
@fergusean fergusean changed the title fix(http): prevent JSON payloads from spoofing UploadedFile fix(http): prevent JSON payloads from spoofing UploadedFile (original) Jul 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants