Skip to content

Add defensive args parsing to stop FUD around this module#2

Merged
WebReflection merged 1 commit into
mainfrom
defensive-arguments
Oct 12, 2024
Merged

Add defensive args parsing to stop FUD around this module#2
WebReflection merged 1 commit into
mainfrom
defensive-arguments

Conversation

@WebReflection

@WebReflection WebReflection commented Oct 12, 2024

Copy link
Copy Markdown
Member

In this unfortunate thread mdn/content#36294 (comment) has been brought up the fact this ungap module is not 100% compliant with native JSON behavior.

Even if it's stated at the top of the ungap documentation that this project doesn't care about paranoia or irrelevant (slow, bloated, etc) extra checks are often not part of the offer:

The main purpose of this project is to help developers move forward, and possibly without unnecessary bloat. This basically means that polyfills are written to support 99% of the use cases, without granting 100% spec compliance.

it was extremely trivial to make it 100% specs compliant and as resilient as core-js counterpart by using (and trapping) once Reflect.apply to guarantee that even in poisoned environment, if this module is imported soon enough everything will just work as expected.

Changes

  • strictly check reviver and replacer to reflect current native JSON expectations
  • trap once and use Reflect.apply to be sure tainted envs can't intercept json values

TODO

  • there are other prototype methods that could be poisoned so that I might just drop Reflect.apply if I find it way slower than just .call and .apply when appropriate ... it's not the goal of this module to be super defensive about everything because even in core-js if somebody manages to poison Function.prototype.call or bind nothing is safe, so I don't like this false claim that any polyfill out there is safer than others, it's always a matter of race conditions nobody can do anything about because that's how JS works: it's scripting, it's dynamic, one can play safe but it never really is (custom browsers with code injected AOT, extensions that might get a chance to taint the env before the page/module does, and so on)
  • measure performance impact of that Reflect.apply and if bad get rid of that false sense of security that gives this temporary module (until all browsers ship it natively) nothing useful at all for the real-world

That's it ... I might as well just drop Reflct.apply already before merging this MR as I don't really like the premises to have it in.

@coveralls

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 11308578147

Details

  • 9 of 9 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 11271610914: 0.0%
Covered Lines: 90
Relevant Lines: 90

💛 - Coveralls

@WebReflection

Copy link
Copy Markdown
Member Author

On the other hand, I can just merge this and move on ... hopefully browsers will implement this soon enough to not worry about performance and Chrome is already shipping this so majority of users will be safe ... I can move on with the next thing, everyone wins, little perf deviation won't matter neither.

Let's publish this as patch 🥳

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.

2 participants