Skip to content

Commit

Permalink
add additional forbidden term for ampproject.org (#39803)
Browse files Browse the repository at this point in the history
* add ampproject.org presubmit check

* fix forbidden ampproject term

* temp?

* single out individual lines to disable forbidden term

* change ampproject.org link to amp.dev

* simplify forbidden term regex for CDN URL
  • Loading branch information
erwinmombay authored Feb 7, 2024
1 parent 461b9d1 commit 190a7ea
Show file tree
Hide file tree
Showing 8 changed files with 15 additions and 6 deletions.
2 changes: 2 additions & 0 deletions ads/_a4a-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ export function getA4ARegistry() {
* @type {!{[key: string]: string}}
*/
export const signingServerURLs = {
/* eslint-disable local/no-forbidden-terms */
'google': 'https://cdn.ampproject.org/amp-ad-verifying-keyset.json',
'google-dev': 'https://cdn.ampproject.org/amp-ad-verifying-keyset-dev.json',
/* eslint-enable local/no-forbidden-terms */
};
2 changes: 1 addition & 1 deletion build-system/server/app-index/template.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ function renderTemplate(opt_params = {}) {
html`
<div class="center">
Built with 💙 by
<a href="https://ampproject.org" class="underlined">the AMP Project</a>.
<a href="https://amp.dev" class="underlined">the AMP Project</a>.
</div>
`,
]);
Expand Down
9 changes: 5 additions & 4 deletions build-system/test-configs/forbidden-terms.js
Original file line number Diff line number Diff line change
Expand Up @@ -919,12 +919,15 @@ const forbiddenTermsSrcInclusive = {
'extensions/amp-consent/0.1/consent-state-manager.js',
],
},
'(cdn|3p)\\.ampproject\\.': {
'ampproject\\.': {
message:
'The CDN domain should typically not be hardcoded in source ' +
'code. Use urls from src/config/urls.js instead.',
allowlist: [
'ads/_a4a-config.js',
// NOTE: Do not allow source code to be allowlisted for this rule.
// explicitly allow individual lines through eslint. The only
// exception is `src/config/urls.js` as it contains the variables
// that need to be referenced by other modules.
'build-system/server/amp4test.js',
'build-system/server/app-index/amphtml-helpers.js',
'build-system/server/app-video-testbench.js',
Expand All @@ -933,8 +936,6 @@ const forbiddenTermsSrcInclusive = {
'build-system/server/variable-substitution.js',
'build-system/tasks/dist.js',
'build-system/tasks/helpers.js',
'src/3p-frame.js',
'src/amp-story-player/amp-story-player-impl.js',
'src/config/urls.js',
'testing/local-amp-chrome-extension/background.js',
'tools/experiments/experiments.js',
Expand Down
2 changes: 2 additions & 0 deletions extensions/amp-access/0.1/amp-access-server-jwt.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,10 @@ const TAG = 'amp-access-server-jwt';
/** @const {number} */
const AUTHORIZATION_TIMEOUT = 3000;

/* eslint-disable local/no-forbidden-terms */
/** @const {string} */
const AMP_AUD = 'ampproject.org';
/* eslint-enable local/no-forbidden-terms */

/**
* This class implements server-side authorization protocol with JWT. In this
Expand Down
2 changes: 2 additions & 0 deletions extensions/amp-iframe/0.1/amp-iframe.js
Original file line number Diff line number Diff line change
Expand Up @@ -155,9 +155,11 @@ export class AmpIframe extends AMP.BaseElement {
userAssert(
!(
endsWith(hostname, `.${urls.thirdPartyFrameHost}`) ||
// eslint-disable-next-line local/no-forbidden-terms
endsWith(hostname, '.ampproject.org')
),
'amp-iframe does not allow embedding of frames from ' +
// eslint-disable-next-line local/no-forbidden-terms
'ampproject.*: %s',
src
);
Expand Down
1 change: 1 addition & 0 deletions src/3p-frame.js
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,7 @@ export function getDevelopmentBootstrapBaseUrl(parentWindow, srcFileBasename) {
*/
function getAdsLocalhost(win) {
let adsUrl = urls.thirdParty; // local dev with a non-localhost server
// eslint-disable-next-line local/no-forbidden-terms
if (adsUrl == 'https://3p.ampproject.net') {
adsUrl = 'http://ads.localhost'; // local dev with a localhost server
}
Expand Down
2 changes: 1 addition & 1 deletion src/amp-story-player/amp-story-player-impl.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ const STORY_POSITION_ENUM = {
};

/** @const @type {!Array<string>} */
const SUPPORTED_CACHES = ['cdn.ampproject.org', 'www.bing-amp.com'];
const SUPPORTED_CACHES = ['cdn.ampproject.org', 'www.bing-amp.com']; // eslint-disable-line local/no-forbidden-terms

/** @const @type {!Array<string>} */
const SANDBOX_MIN_LIST = ['allow-top-navigation'];
Expand Down
1 change: 1 addition & 0 deletions src/cookies.js
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,7 @@ function trySetCookie(
// We do not allow setting cookies on the domain that contains both
// the cdn. and www. hosts.
// Note: we need to allow cdn.ampproject.org in order to optin to experiments
// eslint-disable-next-line local/no-forbidden-terms
if (domain == 'ampproject.org') {
// Actively delete them.
value = 'delete';
Expand Down

0 comments on commit 190a7ea

Please sign in to comment.