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

Remove '\u0000' from input when sanitizing null input #89

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

Conversation

such
Copy link

@such such commented Oct 28, 2024

No description provided.

@whitequark
Copy link
Owner

There's a lot of unrelated formatting chances so I won't reviewing the PR.

@zachyale
Copy link

@whitequark Aside from the formatting changes, do you have an issue with this change? Would like to use this gem for sanitizing but we also need to sanitize \u0000, and happy to put up a PR without needless style changes

@whitequark
Copy link
Owner

I haven't reviewed it due to the formatting changes. I haven't even used Ruby in almost a decade and I don't want to waste my time looking at a PR like this one.

@AlexWayfer
Copy link
Contributor

I haven't even used Ruby in almost a decade and I don't want to waste my time looking at a PR like this one.

It's OK, but maybe you should assign a new maintainer for this repo. If you're not sure who — open an issue for this.

@whitequark
Copy link
Owner

I'm aware of my options.

@such such force-pushed the am/null-byte-string branch from 20d0065 to 7042887 Compare December 16, 2024 16:30
@zachyale
Copy link

@whitequark / @AlexWayfer - the PR has been updated to remove unnecesarry formatting changes. Can we get a review please?

@@ -8,6 +8,7 @@ module Rack
class UTF8Sanitizer
StringIO = ::StringIO
NULL_BYTE_REGEX = /\x00/.freeze
NULL_BYTE_STRING_REGEX = Regexp.new('\\\u0000').freeze
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm quite confused about this PR. Isn't \\\u0000 matching normal text that also happens to be a Ruby string escape sequence? There shouldn't be any reason to remove it from the input unless you're doing something rather strange with the string.

What problem does this actually solve?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even though it's indeed normal text connection.escape(s) fails with PG when a string contains this.

Copy link
Owner

@whitequark whitequark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the stated purpose, but I don't think this should be the responsibility of this library, and I don't think removing \\u0000 is even correct in the general case. (It is perfectly valid UTF-8, after all.)

@such
Copy link
Author

such commented Dec 17, 2024

I understand the stated purpose, but I don't think this should be the responsibility of this library, and I don't think removing \\u0000 is even correct in the general case. (It is perfectly valid UTF-8, after all.)

even if it was optional as well?

@whitequark
Copy link
Owner

This has nothing to do with invalid UTF-8, so yes.

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.

4 participants