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

yarn: replace credentials with dummy creds #9466

Merged
merged 2 commits into from Apr 10, 2024

Conversation

jakecoffman
Copy link
Member

Customers are setting npmAlwaysAuth in their .yarmrc.yml which due to our sanitation results in No authentication configured for request, even if they set a fallback value with ${TOKEN:-fallback} because we clear out the entire thing.

This works around that by setting a default value ourselves.

For the life of me I cannot reproduce this issue locally, but it should be completely harmless assuming all the tests pass.

@jakecoffman jakecoffman requested a review from a team as a code owner April 9, 2024 21:00
@jakecoffman jakecoffman changed the title replace credentials with dummy creds npm: replace credentials with dummy creds Apr 9, 2024
@jakecoffman jakecoffman changed the title npm: replace credentials with dummy creds yarn: replace credentials with dummy creds Apr 9, 2024
@jakecoffman
Copy link
Member Author

I verified this does fix the issue.

@jakecoffman jakecoffman merged commit 15c9b2f into main Apr 10, 2024
65 checks passed
@jakecoffman jakecoffman deleted the jakecoffman/use-dummy-creds branch April 10, 2024 14:31
content.gsub(/\"\$\{.*?\}\"/, '""').gsub(/\$\{.*?\}/, '""')
# Replace all "${...}" and ${...} occurrences with dummy strings. We use
# dummy strings instead of empty strings to prevent issues with npmAlwaysAuth
content.gsub(/"\$\{.*?}"/, '"DUMMYCREDS"').gsub(/\$\{.*?}/, '"DUMMYCREDS"')
Copy link

Choose a reason for hiding this comment

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

I think this regex is no longer catching the closing brace }. We're getting this error:

updater | 2024/04/11 00:22:38 ERROR <job_812946518> bad URI(is not URI?): "https://our-npm-registry.us.online/registry}/api/npm/our-npm/express"

Choose a reason for hiding this comment

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

I'm not particularly familiar with regex sorry, but I think it might be a ruby regex thing where } still needs to be escaped.

I think if the regex check is just reverted to the fully escaped version it will be fixed.

Suggested change
content.gsub(/"\$\{.*?}"/, '"DUMMYCREDS"').gsub(/\$\{.*?}/, '"DUMMYCREDS"')
content.gsub(/\"\$\{.*?\}\"/, '"DUMMYCREDS"').gsub(/\$\{.*?\}/, '"DUMMYCREDS"')

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't see how this PR would have caused that as job 812946518 is failing during the UpdateChecker step which happens before the FileUpdater step where I made this change.

Choose a reason for hiding this comment

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

Ah gotcha. Maybe something wrong on our end? I'll do some digging.

Choose a reason for hiding this comment

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

Yep, looks like that was an issue on our end sorry.

Our repo was trying to do some odd conditional assignment in a .yarnrc.yml:

npmRegistryServer: '${REMOTE_URL:-https://our.url.us.online/remote}/api/npm/our-npm/'

This fixed it:

npmRegistryServer: "https://our.url.us.online/remote/api/npm/our-npm/"

I'll look up the yml syntax guide in the morning and figure out what we were doing wrong. Thanks for your help, greatly appreciated.

Choose a reason for hiding this comment

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

The GitHub documentation says this:

When accessing environment variables in your .yarnrc.yml file, you should provide a fallback value such as ${ENV_VAR-fallback} or ${ENV_VAR:-fallback}.

But with this change, the fallback value provided is no longer used because it's stripped out by sanitize_yarnrc_content (resulting in the above problem with npmRegistryServer). Is this intentional? If so, then the GitHub documentation should be updated because it gives the impression that you can provide a fallback value, when in fact the fallback is always DUMMYCREDS.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it was originally intended when it was added in #9322, but if it simplifies the docs then I think it's a win.

@pavera I see you added the blurb about adding the fallback value. Do you recall it done because of npmAlwaysAuth?

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

Successfully merging this pull request may close these issues.

None yet

4 participants