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

Inconsistencies in CYPRESS_DOWNLOAD_PATH_TEMPLATE handling #19914

Closed
mjhenkes opened this issue Jan 26, 2022 · 4 comments · Fixed by #20698
Closed

Inconsistencies in CYPRESS_DOWNLOAD_PATH_TEMPLATE handling #19914

mjhenkes opened this issue Jan 26, 2022 · 4 comments · Fixed by #20698

Comments

@mjhenkes
Copy link
Member

Current behavior

There seem to be some inconsistencies in how CYPRESS_DOWNLOAD_PATH_TEMPLATE is handled from the .npmrc file depending on package manager and version.

Given
CYPRESS_DOWNLOAD_PATH_TEMPLATE=${endpoint}/${platform}-${arch}/cypress.zip

npm 7 works, though locally i've seen the env fail to get picked up because of casing.
npm 6 attempts to replay endpoint etc with other ENVs (this is unfortunate, but you can escape them but that doesn't work either)
Yarn 1 doesn't remove double quotes.

I haven't tested yarn 3 or npm 8 but they should be looked at, as well as windows useage.

Desired behavior

Ideally you could set the CYPRESS_DOWNLOAD_PATH_TEMPLATE variable with any package manager

Test code to reproduce

Try out setting variations of

CYPRESS_DOWNLOAD_PATH_TEMPLATE=${endpoint}/${platform}-${arch}/cypress.zip
CYPRESS_DOWNLOAD_PATH_TEMPLATE=\${endpoint}/\${platform}-\${arch}/cypress.zip
CYPRESS_DOWNLOAD_PATH_TEMPLATE="${endpoint}/${platform}-${arch}/cypress.zip"
CYPRESS_DOWNLOAD_PATH_TEMPLATE="\${endpoint}/\${platform}-\${arch}/cypress.zip"

Cypress Version

9.3.1

Other

Where CYPRESS_DOWNLOAD_PATH_TEMPLATE is used
https://github.com/cypress-io/cypress/blob/develop/cli/lib/tasks/download.js#L67

getEnv implementation
https://github.com/cypress-io/cypress/blob/develop/cli/lib/util.js#L459

We should probably be setting trim on getENV, that should resolve any double quotes problems.

@amoshydra
Copy link
Contributor

backslash is very uncommon in URL. Would it be reasonable to escape backslash specifically for CYPRESS_DOWNLOAD_PATH_TEMPLATE in current version?

https://github.com/cypress-io/cypress/blob/v9.3.1/cli/lib/tasks/download.js#L69-L72

Proposal:

  const prepend = (urlPath) => {
    const endpoint = Url.resolve(getBaseUrl(), urlPath)
    const platform = os.platform()
    const pathTemplate = util.getEnv('CYPRESS_DOWNLOAD_PATH_TEMPLATE')
  
    return pathTemplate
-     ? pathTemplate.replace('${endpoint}', endpoint).replace('${platform}', platform).replace('${arch}', arch())
+     ? pathTemplate.replace(/\\?\$\{endpoint\}/, endpoint).replace(/\\?\$\{platform\}/, platform).replace(/\\?\$\{arch\}/, arch())
      : `${endpoint}?platform=${platform}&arch=${arch()}`
}

Test snippet

output = (pathTemplate) => {
    const endpoint = 'endpoint';
    const platform = 'platform';
    const arch = () => 'arch';

    const codeFn = () => {
        return pathTemplate
            ? pathTemplate.replace(/\\?\$\{endpoint\}/, endpoint).replace(/\\?\$\{platform\}/, platform).replace(/\\?\$\{arch\}/, arch())
            : `${endpoint}?platform=${platform}&arch=${arch()}`;
    };

    console.log(`input : ${pathTemplate}`);
    console.log(`output: ${codeFn()}`);
};

output("\\${endpoint}/\\${platform}-\\${arch}/cypress.zip");
// input : \${endpoint}/\${platform}-\${arch}/cypress.zip
// output: endpoint/platform-arch/cypress.zip

output("${endpoint}/${platform}-${arch}/cypress.zip");
// input : ${endpoint}/${platform}-${arch}/cypress.zip
// output: endpoint/platform-arch/cypress.zip

@cypress-bot cypress-bot bot added stage: needs review The PR code is done & tested, needs review and removed stage: ready for work The issue is reproducible and in scope labels Mar 19, 2022
@amoshydra
Copy link
Contributor

amoshydra commented Mar 19, 2022

I have done additional research in Windows 10 comparing the behaviour for

  • npm@6
  • npm@7
  • npm@8
  • yarn@1
cli npmrc cypress input
npm@6.14.16 ${endpoint}/${platform}-${arch}/cypress.zip Error: Failed to replace env in config: ${endpoint}
npm@6.14.16 \${endpoint}/\${platform}-\${arch}/cypress.zip \${endpoint}/\${platform}-\${arch}/cypress.zip
npm@6.14.16 "${endpoint}/${platform}-${arch}/cypress.zip" Error: Failed to replace env in config: ${endpoint}
npm@6.14.16 "\${endpoint}/\${platform}-\${arch}/cypress.zip" Error: Failed parsing JSON config key
-- -- --
npm@7.24.2 ${endpoint}/${platform}-${arch}/cypress.zip ${endpoint}/${platform}-${arch}/cypress.zip
npm@7.24.2 \${endpoint}/\${platform}-\${arch}/cypress.zip ${endpoint}/${platform}-${arch}/cypress.zip
npm@7.24.2 "${endpoint}/${platform}-${arch}/cypress.zip" ${endpoint}/${platform}-${arch}/cypress.zip
npm@7.24.2 "\${endpoint}/\${platform}-\${arch}/cypress.zip" "${endpoint}/${platform}-${arch}/cypress.zip"
-- -- --
npm@8.5.5 ${endpoint}/${platform}-${arch}/cypress.zip ${endpoint}/${platform}-${arch}/cypress.zip
npm@8.5.5 \${endpoint}/\${platform}-\${arch}/cypress.zip ${endpoint}/${platform}-${arch}/cypress.zip
npm@8.5.5 "${endpoint}/${platform}-${arch}/cypress.zip" ${endpoint}/${platform}-${arch}/cypress.zip
npm@8.5.5 "\${endpoint}/\${platform}-\${arch}/cypress.zip" "${endpoint}/${platform}-${arch}/cypress.zip"
-- -- --
yarn@1.22.18 ${endpoint}/${platform}-${arch}/cypress.zip Failed to replace env in config: ${endpoint}
yarn@1.22.18 \${endpoint}/\${platform}-\${arch}/cypress.zip \${endpoint}/\${platform}-\${arch}/cypress.zip
yarn@1.22.18 "${endpoint}/${platform}-${arch}/cypress.zip" Failed to replace env in config: ${endpoint}
yarn@1.22.18 "\${endpoint}/\${platform}-\${arch}/cypress.zip" "\${endpoint}/\${platform}-\${arch}/cypress.zip"

* I am not sure how I can test yarn2 or yarn 3 yet.

From the findings above, cypress will need to be able to handle 3 4 cases:

  1. ${endpoint}/${platform}-${arch}/cypress.zip
  2. \${endpoint}/\${platform}-\${arch}/cypress.zip
  3. "\${endpoint}/\${platform}-\${arch}/cypress.zip"
  4. "${endpoint}/${platform}-${arch}/cypress.zip"

All three four cases will be addressed in #20698 @ 6c74734

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Mar 24, 2022

The code for this is done in cypress-io/cypress#20698, but has yet to be released.
We'll update this issue and reference the changelog when it's released.

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Mar 28, 2022

Released in 9.5.3.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v9.5.3, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Mar 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants