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.

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

core(image-elements): do not set untrusted natural dimensions #11457

Merged
merged 2 commits into from
Sep 28, 2020
Merged

core(image-elements): do not set untrusted natural dimensions #11457

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
450e68a
core(image-elements): do not set natural dimensions when in picture/s…
patrickhulce Sep 18, 2020
aab66b7
Update lighthouse-cli/test/fixtures/static-server.js
patrickhulce Sep 18, 2020
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
10 changes: 10 additions & 0 deletions lighthouse-cli/test/fixtures/byte-efficiency/tester.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,16 @@ <h2>Byte efficiency tester page</h2>
<!-- PASS(unsized-images): css position is absolute -->
<img class="onscreen" src="lighthouse-320x212-poor.jpg?duplicate">

<!-- PASS(optimized): image is JPEG optimized -->
<!-- PASS(webp): image has insigificant WebP savings -->
<!-- PASS(responsive): image is used at full size -->
<!-- PASS(responsive-inverse): image accounts for DPR 2.625 in srcset *with a redirect* -->
<!-- PASS(offscreen): image is onscreen -->
<!-- PASS(unsized-images): css position is absolute -->
<!-- The first 320x212 should never be requested by Lighthouse. -->
<!-- 320x212 image just there to ensure failure of other audits if srcset is used incorrectly. -->
<img class="onscreen" width="240" src="lighthouse-320x212-poor.jpg?neverused" srcset="lighthouse-320x212-poor.jpg?neverused 320w, made-up-image-redirect-image.jpg?redirect=lighthouse-480x320.webp 480w">

<!-- PASS(optimized): image is JPEG optimized -->
<!-- FAIL(webp): image is not WebP optimized -->
<!-- PASS(responsive): image is in CSS -->
Expand Down
8 changes: 8 additions & 0 deletions lighthouse-cli/test/fixtures/static-server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,14 @@ class Server {
}

function sendRedirect(url) {
// Redirects can only contain ASCII characters.
if (url.split('').some(char => char.charCodeAt(0) > 256)) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

only semi-related, at one point debugging I put a redirect that contained non-ascii characters and it crashed the server entirely, which is suboptimal. returning a 500 makes more sense :)

patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
response.writeHead(500);
response.write(`Invalid redirect URL: ${url}`);
response.end();
return;
}

const headers = {
Location: url,
};
Expand Down
13 changes: 7 additions & 6 deletions lighthouse-core/gather/gatherers/image-elements.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ function getHTMLImages(allElements) {

return allImageElements.map(element => {
const computedStyle = window.getComputedStyle(element);
const isPicture = !!element.parentElement && element.parentElement.tagName === 'PICTURE';
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
const canTrustNaturalDimensions = !isPicture && !element.srcset;
return {
// currentSrc used over src to get the url as determined by the browser
// after taking into account srcset/media/sizes/etc.
Expand All @@ -65,18 +67,18 @@ function getHTMLImages(allElements) {
displayedWidth: element.width,
displayedHeight: element.height,
clientRect: getClientRect(element),
naturalWidth: element.naturalWidth,
naturalHeight: element.naturalHeight,
naturalWidth: canTrustNaturalDimensions ? element.naturalWidth : 0,
naturalHeight: canTrustNaturalDimensions ? element.naturalHeight : 0,
attributeWidth: element.getAttribute('width') || '',
attributeHeight: element.getAttribute('height') || '',
cssWidth: undefined, // this will get overwritten below
cssHeight: undefined, // this will get overwritten below
cssComputedPosition: getPosition(element, computedStyle),
isCss: false,
isPicture,
// @ts-expect-error: loading attribute not yet added to HTMLImageElement definition.
loading: element.loading,
resourceSize: 0, // this will get overwritten below
isPicture: !!element.parentElement && element.parentElement.tagName === 'PICTURE',
usesObjectFit: ['cover', 'contain', 'scale-down', 'none'].includes(
computedStyle.getPropertyValue('object-fit')
),
Expand Down Expand Up @@ -175,7 +177,7 @@ function determineNaturalSize(url) {
}

/**
* @param {LH.Crdp.CSS.CSSStyle} [style]
* @param {LH.Crdp.CSS.CSSStyle|undefined} style
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

drive by fix, vscode flags this as a ts error

* @param {string} property
* @return {string | undefined}
*/
Expand All @@ -191,7 +193,7 @@ function findSizeDeclaration(style, property) {
/**
* Finds the most specific directly matched CSS font-size rule from the list.
*
* @param {Array<LH.Crdp.CSS.RuleMatch>} [matchedCSSRules]
* @param {Array<LH.Crdp.CSS.RuleMatch>|undefined} matchedCSSRules
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

drive by fix, vscode flags this as a ts error

* @param {string} property
* @returns {string | undefined}
*/
Expand Down Expand Up @@ -346,7 +348,6 @@ class ImageElements extends Gatherer {
// or it's not in the top 50 largest images.
if (
(element.isPicture || element.isCss || element.srcset) &&
networkRecord &&
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

confusion fix, we always default networkRecord to {} if we couldn't find one so this check does nothing

top50Images.includes(networkRecord)
) {
element = await this.fetchElementWithSizeInformation(driver, element);
Expand Down