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

[SC-562] fix: do not fail tracer for malformed uri #49

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mpmiszczyk
Copy link

Failing tracer will be detached which can brake tracing all together.

Failing tracer will be detached which can brake tracing all together.
@sourcelevel-bot
Copy link

Hello, @mpmiszczyk! This is your first Pull Request that will be reviewed by SourceLevel, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

@GregMefford
Copy link
Member

Hey! Thanks so much for taking the time to contribute this PR! ❤️ 🚀

I am ready to merge it essentially as it is, but there are some minor merge conflicts and I don't have permission to push the rebased changes to your branch. Could you please do a rebase onto master to get it up to date? There's just a test that was removed in a previous PR that you'll need to also remove when you resolve the conflicts in that file.

Alternatively, if you see the option to allow maintainers to push to your branch, you can turn that on and I can update it myself.

@Sija
Copy link

Sija commented Apr 28, 2022

I'm having a difficulty in understanding why this (PR) would be needed at all.

  1. URI.decode shouldn't raise
  2. auth%%27%20AND%202*3*8=6*8%20AND%20%27zPT3%27!=%27zPT3% is not no an URI, but a a path, and still URI.decode wouldn't raise given this as an argument
  3. Invalid paths should be passed along without alteration, otherwise we're losing important information

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