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 #460

Merged
merged 3 commits into from
Oct 1, 2023

Conversation

fergusean
Copy link
Contributor

@fergusean fergusean commented Jul 2, 2023

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.

This is an alternate implementation to #459, as #459 does not work when the HttpBody/HttpBodyValidation type argument is a complex type. The typeof UploadedFileSymbol | null union resolves the issue described here.

My preference would have been to use a Symbol as a key so that it doesn't show up in JSON serialization or IntelliSense, but I couldn't get the symbol to work through deserialization, so even valid uploads would fail post-deserialization validation.

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.

@fergusean fergusean changed the title fix(http): prevent JSON payloads from spoofing UploadedFile (alt. impl.) fix(http): prevent JSON payloads from spoofing UploadedFile Jul 2, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jul 2, 2023

Codecov Report

Merging #460 (a2509a0) into master (3fd48ea) will increase coverage by 0.08%.
The diff coverage is 100.00%.

❗ Current head a2509a0 differs from pull request most recent head 1b4fdee. Consider uploading reports for the commit 1b4fdee 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     #460      +/-   ##
==========================================
+ Coverage   78.37%   78.45%   +0.08%     
==========================================
  Files         173      173              
  Lines       18538    18542       +4     
  Branches     4843     4843              
==========================================
+ Hits        14529    14548      +19     
+ Misses       4009     3994      -15     
Impacted Files Coverage Δ
packages/http/src/request-parser.ts 97.17% <100.00%> (+1.12%) ⬆️
packages/http/src/router.ts 88.64% <100.00%> (+0.14%) ⬆️

... and 2 files with indirect coverage changes

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

@marcj marcj merged commit 1a57c47 into deepkit:master Oct 1, 2023
@marcj
Copy link
Member

marcj commented Oct 1, 2023

Good catch, thank you!

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.

3 participants