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

Relax CORS restrictions for module imports #2549

Merged
merged 2 commits into from
Jul 15, 2024

Conversation

yesil
Copy link
Contributor

@yesil yesil commented Jul 9, 2024

Milo consumers CC/DC are already running
without strict CORS restriction.
This change will enable the feature in PR#2544
to be used on Milo pages as well.

For more context, see: #2544 (comment)

Resolves: MWPW-153962

Test URLs:

Milo consumers CC/DC are already running
without CORS restriction.
This change will enable the feature in PR#2544
to be used on Milo pages as well.
Copy link
Contributor

aem-code-sync bot commented Jul 9, 2024

Page Scores Audits Google
/?martech=off PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

Copy link

codecov bot commented Jul 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.82%. Comparing base (c896518) to head (bd885a6).

Additional details and impacted files
@@            Coverage Diff             @@
##            stage    #2549      +/-   ##
==========================================
- Coverage   95.83%   95.82%   -0.01%     
==========================================
  Files         176      176              
  Lines       46122    46122              
==========================================
- Hits        44199    44195       -4     
- Misses       1923     1927       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yesil yesil added the do not merge PR should not be merged yet label Jul 9, 2024
@yesil
Copy link
Contributor Author

yesil commented Jul 9, 2024

Closing for now, will re-open if needed.

@yesil yesil closed this Jul 9, 2024
@yesil
Copy link
Contributor Author

yesil commented Jul 9, 2024

re-opening for the following reason:

Given:
Milo: https://mwpw-153962--milo--yesil.hlx.live/drafts/ilyas/checkout-links?maslibs=main
CC: https://main--cc--adobecom.hlx.live/products/catalog?milolibs=mwpw-153962--milo--yesil&maslibs=main

Milo one fails with the following error:

checkout-links:1 Access to script at 'https://main--mas--adobecom.hlx.live/lib/commerce.js' from origin 'https://mwpw-153962--milo--yesil.hlx.live' has been blocked by CORS policy: The value of the 'Access-Control-Allow-Origin' header in the response must not be the wildcard '*' when the request's credentials mode is 'include'.

@yesil yesil reopened this Jul 9, 2024
@yesil yesil changed the title Remove CORS restrictions for module imports Relax CORS restrictions for module imports Jul 9, 2024
@yesil yesil removed the do not merge PR should not be merged yet label Jul 9, 2024
Copy link
Contributor

@mokimo mokimo left a comment

Choose a reason for hiding this comment

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

I don't have the context on why we have the flag set to begin with. However, I don't think we pass on any credentials to scripts so this should be fine to remove. 👍

Might have been legacy, as helix has seen some CORS/header changes over the years since this was implemented.

The head.html might have some restrictions in terms of caching, so it might even need some manual flushing of the cache - but we will see.

@mokimo
Copy link
Contributor

mokimo commented Jul 11, 2024

@auniverseaway confirmed on slack that this only exists for legacy reasons and should be removed. It simply wasn't done so far, as it wasn't affecting anyone.

@yesil
Copy link
Contributor Author

yesil commented Jul 11, 2024

@afmicka this is not affected to commerce, but can you perform a smoke test to validate? Thanks.

@afmicka afmicka added the run-nala Run Nala Test Automation against PR label Jul 11, 2024
@afmicka afmicka added verified PR has been E2E tested by a reviewer Ready for Stage labels Jul 11, 2024
@milo-pr-merge
Copy link
Contributor

milo-pr-merge bot commented Jul 11, 2024

Skipped merging 2549: Relax CORS restrictions for module imports due to failing checks

@afmicka
Copy link
Contributor

afmicka commented Jul 11, 2024

@yesil @npeltier @overmyheadandbody @narcis-radu it says that it skipped merging due to the failed checks. Do you know what failing checks? The last commit is green.

@mokimo
Copy link
Contributor

mokimo commented Jul 12, 2024

@afmicka I dug into this and this is very weird. Seems like the same test ran twice at the same time, with one finishing 4 seconds after the other one and failing. Usually, the LASTEST test run is accounted for, so our milo_pr_merge bot is indeed more correct than the UI.

    {
      id: 27332787302,
      name: 'Running E2E & IT',
      status: 'completed',
      conclusion: 'success',
      started_at: '2024-07-11T15:57:43Z',
      completed_at: '2024-07-11T16:02:02Z',
      ...
    },
    {
      id: 27332787513,
      name: 'Running E2E & IT',
      status: 'completed',
      conclusion: 'failure',
      started_at: '2024-07-11T15:57:43Z',
      completed_at: '2024-07-11T16:02:06Z',
    },

So after this I re-ran the nala tests and now they are properly failing

@afmicka
Copy link
Contributor

afmicka commented Jul 12, 2024

Thanks @mokimo. These are now new failures, started in the last hour. They have not been here yesterday in any of the PRs, nor today during European day. Our changes do not affect marketo test pages and are failing in all pur new PRs. Will check

@milo-pr-merge milo-pr-merge bot merged commit a6dbcc6 into adobecom:stage Jul 15, 2024
18 of 19 checks passed
@milo-pr-merge milo-pr-merge bot mentioned this pull request Jul 15, 2024
rohitsahu pushed a commit to rohitsahu/milo that referenced this pull request Jul 16, 2024
* stage:
  Mwpw-142267:  Merch What's Included and Merch Mnemonic List (TwP) (adobecom#2554)
  MWPW-149124 Improve Focus Page for Performance Improvement Tiger Team  (adobecom#2391)
  Correctly send the created PR slacks (adobecom#2566)
  MWPW-153167: caas-config change to enable hiding date detail information (adobecom#2553)
  MWPW-152016 - Localization target preview for transcreation (adobecom#2564)
  Relax CORS restrictions for module imports (adobecom#2549)
  MWPW-153600 [PEP] loader bar on PEP prompt is seen loaded Left to right in RTL locale (adobecom#2548)
  MWPW-146962 [milo] Text link behaving like button in FAQ section (adobecom#2530)
  MWPW-152280 MEP: Only preload fragments that are in the 1st section (adobecom#2525)
  MWPW-152697 Fix Marketo mobile horizontal scroll (adobecom#2514)
  MWPW-151513: Search results vanished when user clicks on Marquee CTA:Start free trail (adobecom#2406)
  MWPW-154013 PEP prompt redirection is broken in stage after the PEP dismissal PR merge (adobecom#2547)
  MWPW-153962: Introduce maslibs query parameter (adobecom#2544)
  Central georouting support (adobecom#2531)
  [MWPW-152278] Avoid empty CSS requests (adobecom#2524)
  MWPW-152918 Fix Marketo button font (adobecom#2513)

# Conflicts:
#	libs/deps/merch-card.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready for Stage run-nala Run Nala Test Automation against PR verified PR has been E2E tested by a reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants