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

fix(smithy-client): remove capturing groups from date regex #3008

Merged
merged 1 commit into from
Nov 10, 2021

Conversation

adamthom-amzn
Copy link
Contributor

Issue

#2977
#2866

Description

A babel plugin is altering regexes with named capturing groups in such a way that renders them invalid.

Testing

Locally in an amplify app, and with unit tests.


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@adamthom-amzn adamthom-amzn requested a review from a team as a code owner November 10, 2021 00:48
@AllanZhengYP
Copy link
Contributor

AllanZhengYP commented Nov 10, 2021

More details of the root cause for both of the issue:

We found in the production build of Create-React-App, Babal will intercept the RegExp if it uses named matching. The new RegExp() will be transfered by babel-plugin-transform-named-capturing-groups-regex. For example, The Regex in the source

const RFC3339 = new RegExp(
/^(?<Y>\d{4})-(?<M>\d{2})-(?<D>\d{2})[tT](?<H>\d{2}):(?<m>\d{2}):(?<s>\d{2})(?:\.(?<frac>\d+))?[zZ]$/
);

are transfered into something like below in uglified code:

var mt=new RegExp(ht(/^([0-9]{4})\x2D([0-9]{2})\x2D([0-9]{2})[Tt]([0-9]{2}):([0-9]{2}):([0-9]{2})(?:\.([0-9]+))?[Zz]$/,{Y:1,M:2,D:3,H:4,m:5,s:6,frac:7}))

And this code cannot match the updated regex.

This issue only affects the production code that uses the mentioned Babel plugin, which is used by default in Create-React-App.

So the fix is removing the named matching from the regex. We've validated it fixes the issue in Create-React-App.

@AllanZhengYP
Copy link
Contributor

AllanZhengYP commented Nov 18, 2021

@adamthom-amzn After digging more on the root cause of the issue, the root cause above in inexact. The original code uses RegExp literal in the new RegExp() constructor. But the babel-plugin-transform-named-capturing-groups-regex plugin only takes care of RegExp literal without RegExp() constructor. For example:

const RFC3339 = /^(?<Y>\d{4})-(?<M>\d{2})-(?<D>\d{2})[tT](?<H>\d{2}):(?<m>\d{2}):(?<s>\d{2})(?:\.(?<frac>\d+))?[zZ]$/;

However here, the original code used the constructor, so the mentioned Babel plugin won't fully kick in. Babel only removes the capturing name <Y>.

Furthermore, RegExp literals in RegExp constructor is only supported since ES 6(link). Most(with default browser list 0.2%) of the React app will transpile the source code to ES5.

A more reasonable fix for it could have been using string literals instead:

const RFC3339 =  new RegExp("^(?<Y>\\d{4})-(?<M>\\d{2})-(?<D>\\d{2})[tT](?<H>\\d{2}):(?<m>\\d{2}):(?<s>\\d{2})(?:\\.(?<frac>\\d+))?[zZ]$");

I have verified it works. But since the current fix also works, I won't revert the change.

@github-actions
Copy link

github-actions bot commented Dec 3, 2021

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants