Skip to content

Conversation

@mernen
Copy link
Contributor

@mernen mernen commented Nov 2, 2010

About the proposed patch: replacing the precise instances of "</script>" via a post-generation scan (like a foo.gsub!("</script>", '<\/script>')) would pretty much defeat the speed purposes of this gem (on the ext/jruby side), and replacing these instances mid-generation (through a look-ahead, for instance) would complicate the code too much. On the other hand, replacing every single "/" character with a "/" sequence could make certain strings needlessly less readable (for instance, URLs starting with "http://"). So, I think the compromise of replacing all "</" sequences with "</" offers the best combination of code simplicity and JSON readability. This patch implements this approach on all 3 versions (pure, ext, jruby) and adds a new test.

@larsklevan
Copy link

+1 for this patch. If nothing else would be great to somehow include it in the Rails compatibility mode because the Rails implementation avoids this issue by unicode escaping the ">".

nurse pushed a commit to nurse/json that referenced this pull request Jul 8, 2011
This should fix issue ruby#51 on Windows
@nurse
Copy link
Member

nurse commented Aug 30, 2011

RFC JSON doesn't allow such extra escape. This patch should be rejected.

Anyway as Rails does, using Unicode escape like str.gsub(/</(script)>/i, "<\u002F\1>") is not bad.

@mernen
Copy link
Contributor Author

mernen commented Aug 30, 2011

I don't see what's not allowed. As the spec says, "Any character may be escaped" (save for the 3 exceptions mentioned a paragraph before, which must be escaped), and the "/" ("solidus") is in the list of two-character escape sequences. Selectively escaping / with \/ seems perfectly valid then.

@nurse
Copy link
Member

nurse commented Aug 30, 2011

Ah, yes / is collect sorry

                %x2F /          ; /    solidus         U+002F

@djvirgen
Copy link

+1

Hoping to see this patch pulled in soon!

@marcandre
Copy link
Member

This should be closed in favor of #405

@marcandre
Copy link
Member

@hsbt could you please close this?

@hsbt hsbt closed this Jul 8, 2020
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.

6 participants