Skip to content

fix(core): don't serve in-page POST/unsafe requests from the discovery cache #2325

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.

Sign up for GitHub

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

Open
Sriram567 wants to merge 1 commit into
base: master
Choose a base branch
from
+36 −1
Open

fix(core): don't serve in-page POST/unsafe requests from the discovery cache #2325

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion packages/core/src/network.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -668,6 +668,15 @@ async function sendResponseResource(network, request, session) {
let meta = { ...network.meta, url };
let send = (method, params) => network.send(session, method, params);

// The resource cache is keyed by URL only, and cached bodies are GET responses.
// Serving one for an unsafe method (POST/PUT/PATCH/DELETE) would answer a
// state-changing request with stale content and stop it from ever reaching the
// origin — e.g. an in-page login POST issued from an execute script would be
// fulfilled from the cached login page and never authenticate (PER-9766). Only
// safe, cacheable methods may be served from cache; anything else (or an absent
// method) safely falls through to a real origin request.
let cacheableMethod = request.method === 'GET' || request.method === 'HEAD';

try {
let resource = network.intercept.getResource(url, network.intercept.currentWidth);
network.log.debug(`Handling request: ${url}`, meta);
Expand All @@ -684,7 +693,7 @@ async function sendResponseResource(network, request, session) {
requestId: request.interceptId,
errorReason: 'Aborted'
});
} else if (resource && (resource.root || resource.provided || !disableCache)) {
} else if (resource && cacheableMethod && (resource.root || resource.provided || !disableCache)) {
// Don't rename the below log line as it is used in getting network logs in api
log.debug(resource.root ? '- Serving root resource' : '- Resource cache hit', meta);

Expand Down
26 changes: 26 additions & 0 deletions packages/core/test/discovery.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2560,6 +2560,32 @@
let nonRootResources = Array.from(percy[RESOURCE_CACHE_KEY].values()).filter(resource => !resource.root);
expect(nonRootResources.length).toEqual(2);
});

it('serves cached resources only for cacheable methods (PER-9766)', async () => {
// A resource is cached on the initial GET during discovery. An in-page POST to
// that same URL (e.g. a login form submitted from an execute script) must reach
// the origin instead of being answered from the URL-keyed cache — otherwise the
// request never authenticates and the snapshot captures unauthenticated content.
let methods = [];
server.reply('/cached.css', req => (methods.push(req.method), [200, 'text/css', '.a{}']));
server.reply('/', () => [200, 'text/html', dedent`
<html><head><link rel="stylesheet" href="/cached.css"/></head>
<body><p>auth</p></body></html>`
]);

await percy.snapshot({
name: 'post bypasses cache',
url: 'http://localhost:8000',
// execute runs after initial discovery, so /cached.css is already cached here
execute: async () => { await fetch('/cached.css', { method: 'POST' }); }

Check failure on line 2580 in packages/core/test/discovery.test.js

View workflow job for this annotation

GitHub Actions / Lint 

'fetch' is not defined
});
await percy.idle();

// GET is cached on first load; the POST must NOT be served from cache, so the
// origin sees it too. On the buggy path the POST was a cache hit and never arrived.
expect(methods).toContain('GET');
expect(methods).toContain('POST');
});
});

describe('with --max-cache-ram', () => {
Expand Down
Loading