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(parseURL, hasProtocol, isScriptProtocol): ignore leading whitespaces #170

Merged
merged 20 commits into from Aug 24, 2023

Conversation

danielroe
Copy link
Member

@danielroe danielroe commented Aug 24, 2023

πŸ”— Linked issue

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

A couple of linked fixes with parsing protocols in URLs: https://url.spec.whatwg.org/#scheme-state

Thanks to @OhB00.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@danielroe danielroe added the bug Something isn't working label Aug 24, 2023
@danielroe danielroe requested a review from pi0 August 24, 2023 14:44
@danielroe danielroe self-assigned this Aug 24, 2023
@codecov
Copy link

codecov bot commented Aug 24, 2023

Codecov Report

Merging #170 (2e93dfb) into main (e600fc0) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 2e93dfb differs from pull request most recent head 6b58388. Consider uploading reports for the commit 6b58388 to get more accurate results

@@           Coverage Diff           @@
##             main     #170   +/-   ##
=======================================
  Coverage   94.91%   94.92%           
=======================================
  Files           7        7           
  Lines         846      847    +1     
  Branches      177      177           
=======================================
+ Hits          803      804    +1     
  Misses         43       43           
Files Changed Coverage Ξ”
src/parse.ts 100.00% <100.00%> (ΓΈ)
src/utils.ts 99.13% <100.00%> (ΓΈ)

src/parse.ts Outdated Show resolved Hide resolved
@pi0 pi0 changed the title fix: ignore whitespace when detecting protocol fix(parseURL, hasProtocol): ignore leading whitespaces Aug 24, 2023
@pi0
Copy link
Member

pi0 commented Aug 24, 2023

Thanks for the PR @danielroe ❀️ I have pushed a few more fixes to always normalize/remove/ignore leading whitespaces as per spec and additional tests.

I am still worried about hasProcol and isScriptProtocol utils (silently) saying a bad input having protocol with a leading space. A better solution could be throwing an explicit error but since all our utils don't have an error let's see if it causes any further security issues. (i can't tell because we have currently no report scenarios)


Also thanks for reporting security issues as always @OhB00 ❀️ Please mention if you see any more possible issues with leading whitespace handling Also please consider reporting issues according to security policy (or discord!) so that we can have a chance to internally properly discuss possible actions before public disclosure.

@pi0 pi0 changed the title fix(parseURL, hasProtocol): ignore leading whitespaces fix(parseURL, hasProtocol, isScriptProtocol): ignore leading whitespaces Aug 24, 2023
@pi0 pi0 merged commit c9e385e into main Aug 24, 2023
2 checks passed
@pi0 pi0 deleted the fix/protocol-whitespace branch August 24, 2023 18:09
@OhB00
Copy link

OhB00 commented Aug 24, 2023

Thanks for the PR @danielroe ❀️ I have pushed a few more fixes to always normalize/remove/ignore leading whitespaces as per spec and additional tests.

I am still worried about hasProcol and isScriptProtocol utils (silently) saying a bad input having protocol with a leading space. A better solution could be throwing an explicit error but since all our utils don't have an error let's see if it causes any further security issues. (i can't tell because we have currently no report scenarios)


Also thanks for reporting security issues as always @OhB00 ❀️ Please mention if you see any more possible issues with leading whitespace handling Also please consider reporting issues according to security policy (or discord!) so that we can have a chance to internally properly discuss possible actions before public disclosure.

Apologies for not disclosing on Huntr, I did not originally view this as a security issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants