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 the issue that prevents password-protected storefronts with an ampersand & from logging in #4657

Merged
merged 1 commit into from
Oct 18, 2024

Conversation

karreiro
Copy link
Contributor

WHY are these changes introduced?

Fixes #4597 and improves the password prompt message (used in shopify theme console/shopify theme dev/shopify app dev) to clarify which password the prompt refers to.

WHAT is this pull request doing?

This PR fixes the issue by encoding passwords when checking if they are valid.

How to test your changes?

  • Update your store to use an ampersand & in the password.
  • Run git cherry-pick 3f5c201cc7d2c23259ffcccd2e217a3ba3b94644 (this will clear your password, so you will be able to see the prompt even if your password is cached).
  • Run shopify theme console.
  • Notice you can log in and use the command.
  • Notice as well that messages present the password page, so there's no more ambiguity about the password being asked.

Screenshot 2024-10-16 at 13 36 31

Screenshot 2024-10-16 at 13 36 26

Post-release steps

N/A

Measuring impact

How do we know this change was effective? Please choose one:

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
  • Existing analytics will cater for this addition
  • PR includes analytics changes to measure impact

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes

…persand `&` from logging in, and improve the password prompt message to disambiguate passwords
@karreiro karreiro requested review from a team as code owners October 16, 2024 11:52
Copy link
Contributor

Differences in type declarations

We detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:

  • Some seemingly private modules might be re-exported through public modules.
  • If the branch is behind main you might see odd diffs, rebase main into this branch.

New type declarations

We found no new type declarations in this PR

Existing type declarations

packages/cli-kit/dist/public/node/themes/urls.d.ts
@@ -3,4 +3,5 @@ import { AdminSession } from '@shopify/cli-kit/node/session';
 export declare function themePreviewUrl(theme: Theme, session: AdminSession): string;
 export declare function themeEditorUrl(theme: Theme, session: AdminSession): string;
 export declare function codeEditorUrl(theme: Theme, session: AdminSession): string;
-export declare function storeAdminUrl(session: AdminSession): string;
\ No newline at end of file
+export declare function storeAdminUrl(session: AdminSession): string;
+export declare function storePasswordPage(store: AdminSession['storeFqdn']): string;
\ No newline at end of file
packages/cli-kit/dist/private/node/ui/components/TextPrompt.d.ts
@@ -2,7 +2,7 @@ import { InlineToken, TokenItem } from './TokenizedText.js';
 import { AbortSignal } from '../../../../public/node/abort.js';
 import { FunctionComponent } from 'react';
 export interface TextPromptProps {
-    message: string;
+    message: TokenItem;
     onSubmit: (value: string) => void;
     defaultValue?: string;
     password?: boolean;

Copy link
Contributor

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
72.51% (+0.01% 🔼)
8365/11537
🟡 Branches
69.15% (+0.02% 🔼)
4105/5936
🟡 Functions
71.78% (+0.01% 🔼)
2187/3047
🟡 Lines
72.8% (+0.01% 🔼)
7908/10862

Test suite run success

1904 tests passing in 866 suites.

Report generated by 🧪jest coverage report action from 1fe7024

Copy link
Contributor

@EvilGenius13 EvilGenius13 left a comment

Choose a reason for hiding this comment

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

Awesome. Thank you for the fix! 👍

Copy link
Contributor

@jamesmengo jamesmengo left a comment

Choose a reason for hiding this comment

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

🎩 LGTM!

image

Looks much cleaner in terminals that support hyperlinks.

I wonder if there's a way to control the whitespace / new lines when rendering. Probably part of cli-kit 🤔

Either way, thanks for this!

const response = await fetch(`${prependHttps(store)}/password`, {
headers: {
'cache-control': 'no-cache',
'content-type': 'application/x-www-form-urlencoded',
},
body: `form_type=storefront_password&utf8=%E2%9C%93&password=${password}`,
body: params.toString(),
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

const response = await fetch(`${prependHttps(store)}/password`, {
headers: {
'cache-control': 'no-cache',
'content-type': 'application/x-www-form-urlencoded',
},
body: `form_type=storefront_password&utf8=%E2%9C%93&password=${password}`,
body: params.toString(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch 😄

@karreiro
Copy link
Contributor Author

@Shopify/app-inner-loop Could you please approve this PR?

@karreiro karreiro added this pull request to the merge queue Oct 18, 2024
Merged via the queue into main with commit a28f707 Oct 18, 2024
36 checks passed
@karreiro karreiro deleted the fix-4597 branch October 18, 2024 15:08
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.

[Bug]: Shopify cli password input difficult to enter into the prompt
6 participants