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

html search: use a Map to collect file-term scores #13060

Merged

Conversation

jayaddison
Copy link
Contributor

@jayaddison jayaddison commented Oct 22, 2024

Feature or Bugfix

  • Bugfix

Purpose

  • Prevent carefully-crafted search queries on the frontend from polluting the scoreMap, potentially allowing for undesired result scoring adjustments.

Detail

  • As recommended in the OWASP Prototype Pollution Cheat-Sheet, use a JavaScript Map instead of an object literal to record per-file term scoring.
  • Use the Map.set method instead of assigning to the properties of the map object.

Relates

  • N/A

Edit: add note about using Map.set in preference to object-property assignment.

@jayaddison jayaddison added html search javascript Pull requests that update Javascript code labels Oct 22, 2024
@jayaddison
Copy link
Contributor Author

My premise, that this is an exploitable problem, seems to be flawed, which is fortunate.

Even so, I don't think that I approached resolving this in a good way; there is a disclosure path for vulnerabilities in Sphinx, and I should have used that.

I did weigh up a few factors about the possible impact of the problem, and then decided to open a pull request without following the disclosure path, but in hindsight that wasn't really a great idea (despite the fact that I now believe the flaw is a non-concern).

I think I'll probably step back from contributing for a while, perhaps here and elsewhere; this seemed to be a hasty effort, and I'd like to make sure I'm being thorough especially about poentially-important problems like security bugs (I believe I have been thorough in the past, but wasn't with this, so that seems to reflect changes in behaviour).

@AA-Turner
Copy link
Member

Even so, I don't think that I approached resolving this in a good way; there is a disclosure path for vulnerabilities in Sphinx, and I should have used that.

I think I'll probably step back from contributing for a while, perhaps here and elsewhere; this seemed to be a hasty effort, and I'd like to make sure I'm being thorough especially about poentially-important problems like security bugs (I believe I have been thorough in the past, but wasn't with this, so that seems to reflect changes in behaviour).

James -- I don't currently have time to review the substantive discussion, but I wanted to quickly write something. Your efforts are immensely appreciated, and I don't want you to take this too hard or etc. Problems in process affect all of us, and so I wouldn't want to loose you from the project (or open source in general!) for a (potential) misstep. Identifying potential problems in the first place is inherently valuable. I sympathise with the feeling of discovering a security problem and wanting to alert people as quickly as possible.

The project can improve here by adding a more advertised SECURITY.rst policy, which I will take as an action. Currently, we use GitHub's Security Advisories.

Please do take all the time you need, but I wanted to write a note to say that I hope you don't overburden yourself with anything and that we keep seeing you around here.

Adam

CHANGES.rst Outdated Show resolved Hide resolved
@AA-Turner AA-Turner changed the title search: use a Map instead of an object literal to collect file-term scores html search: use a Map to collect file-term scores Oct 24, 2024
@AA-Turner AA-Turner merged commit bd7d595 into sphinx-doc:master Oct 24, 2024
24 checks passed
@jayaddison jayaddison deleted the js-security/search-prototype-pollution branch October 24, 2024 18:32
@jayaddison
Copy link
Contributor Author

Thanks again - I think I'll take that time away to recharge/recuperate soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
html search javascript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants