Escape closing tags in strings #51

Open
wants to merge 5 commits into
from

4 participants

@mernen

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

+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 nurse pushed a commit to nurse/json that referenced this pull request Jul 8, 2011
@flori Support building of fat binary gem
This should fix issue #51 on Windows
514d791
@nurse

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

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

Ah, yes \/ is collect sorry

                %x2F /          ; /    solidus         U+002F
@djvirgen

+1

Hoping to see this patch pulled in soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment