Skip to content

Fix multi-delimiter split to get status app token #3505

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.

Sign up for GitHub

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

Jump to bottom
Merged
nateberkopec merged 1 commit into from
Jan 27, 2025
Merged

Fix multi-delimiter split to get status app token #3505

nateberkopec merged 1 commit into from
Jan 27, 2025
+9 −1

Conversation

@AnthonyClark
Copy link
Contributor

@AnthonyClark AnthonyClark commented Sep 28, 2024 •
edited
Loading

Description

Originally added back in a91d64a - I believe the intention is to support splitting on either & or ; between query params.

If nobody has run into a problem with this maybe it can be simplified down to only support the single param, but as I stumbled across it reading through the code I thought I'd propose this change as something simple.

I did hit a CI error due to hotwired/turbo-rails#681 in the ubuntu-20.04 Ruby 2.7 Rails 6.1 run Heh, of course I was just behind on the rebase and it was already taken care of.

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

@MSP-Greg
Copy link
Member

MSP-Greg commented Sep 28, 2024

Re the CI failure, maybe rebase on current master?

If nobody has run into a problem with this

A quick look seemed to show that Puma::App::Status only supports one query parameter (token), so not sure about this?

@AnthonyClark AnthonyClark force-pushed the status-app-token-param-parsing branch from f9fd2a2 to b1b5a04 Compare September 28, 2024 18:37
@AnthonyClark
Copy link
Contributor Author

AnthonyClark commented Sep 28, 2024 •
edited
Loading

only supports one query parameter (token), so not sure about this?

Yeah, I think there's the argument to just remove the split logic - only support the single param with an exact match check on the query param path to be explicit that's what is supported. Would that be the preferred version? (maybe throwing in a secure compare?)

Something like Rack::Utils.secure_compare(env['QUERY_STRING'].to_s, "token=#{@auth_token}") ?

@dentarg
Copy link
Member

dentarg commented Oct 4, 2024

That could be a breaking change if people for some reason had other things in the query string. I agree with using secure comparison but I think we can afford to properly parse the query string, and extract the token value, this isn't some hot path, right?

@MSP-Greg MSP-Greg marked this pull request as ready for review November 10, 2024 20:19
@MSP-Greg
Copy link
Member

MSP-Greg commented Nov 10, 2024

I just checked, and the added test fails in master.

@nateberkopec nateberkopec merged commit 41e2744 into puma:master Jan 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MSP-Greg MSP-Greg MSP-Greg approved these changes

Assignees

No one assigned

Labels

refactor

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@AnthonyClark @MSP-Greg @dentarg @nateberkopec