From 6129adfa28d5e5b6dec74e5ef5a1018f688bd38b Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Fri, 8 Sep 2023 16:07:56 -0700 Subject: [PATCH] Avoid relying on buggy import order in the legacy importer (#245) Historically, the legacy importer shim has strongly assumed that all calls to `canonicalize()` would doubled: once as a URL resolved relative to the current importer, and again as a URL passed to the importers list. However, this relies on buggy, non-spec-compliant behavior in Dart Sass: absolute URLs should never be passed to the current importer, only to the global list of importers. This change avoids relying on that behavior by instead disambiguating relative loads using a custom URL protocol prefix. All canonical URLs passed to Dart Sass now have the prefix `legacy-importer-` added to the beginning. This allows us to disambiguate relative loads, and ignore them for non-`file:` URLs, since relative loads and _only_ relative loads will have schemes beginning with `legacy-importer-`. To avoid regressing the developer experience, we then strip these prefixes from all URLs before they're passed to any developer-facing APIs or displayed to the user. --- lib/src/compile.ts | 62 +++++++++++++++++++++++++++---------- lib/src/legacy/importer.ts | 63 +++++++++++++++++++------------------- lib/src/legacy/index.ts | 55 ++++++++++++++++++--------------- lib/src/legacy/utils.ts | 59 +++++++++++++++++++++++++++++++++++ 4 files changed, 167 insertions(+), 72 deletions(-) create mode 100644 lib/src/legacy/utils.ts diff --git a/lib/src/compile.ts b/lib/src/compile.ts index c60cbf57..96d3a0a2 100644 --- a/lib/src/compile.ts +++ b/lib/src/compile.ts @@ -18,11 +18,32 @@ import {MessageTransformer} from './message-transformer'; import {PacketTransformer} from './packet-transformer'; import {SyncEmbeddedCompiler} from './sync-compiler'; import {deprotofySourceSpan} from './deprotofy-span'; -import {legacyImporterProtocol} from './legacy/importer'; +import { + removeLegacyImporter, + removeLegacyImporterFromSpan, + legacyImporterProtocol, +} from './legacy/utils'; + +/// Allow the legacy API to pass in an option signaling to the modern API that +/// it's being run in legacy mode. +/// +/// This is not intended for API users to pass in, and may be broken without +/// warning in the future. +type OptionsWithLegacy = Options & { + legacy?: boolean; +}; + +/// Allow the legacy API to pass in an option signaling to the modern API that +/// it's being run in legacy mode. +/// +/// This is not intended for API users to pass in, and may be broken without +/// warning in the future. +type StringOptionsWithLegacy = + StringOptions & {legacy?: boolean}; export function compile( path: string, - options?: Options<'sync'> + options?: OptionsWithLegacy<'sync'> ): CompileResult { const importers = new ImporterRegistry(options); return compileRequestSync( @@ -34,7 +55,7 @@ export function compile( export function compileString( source: string, - options?: StringOptions<'sync'> + options?: StringOptionsWithLegacy<'sync'> ): CompileResult { const importers = new ImporterRegistry(options); return compileRequestSync( @@ -46,7 +67,7 @@ export function compileString( export function compileAsync( path: string, - options?: Options<'async'> + options?: OptionsWithLegacy<'async'> ): Promise { const importers = new ImporterRegistry(options); return compileRequestAsync( @@ -58,7 +79,7 @@ export function compileAsync( export function compileStringAsync( source: string, - options?: StringOptions<'async'> + options?: StringOptionsWithLegacy<'async'> ): Promise { const importers = new ImporterRegistry(options); return compileRequestAsync( @@ -151,7 +172,7 @@ function newCompileRequest( async function compileRequestAsync( request: proto.InboundMessage_CompileRequest, importers: ImporterRegistry<'async'>, - options?: Options<'async'> + options?: OptionsWithLegacy<'async'> & {legacy?: boolean} ): Promise { const functions = new FunctionRegistry(options?.functions); const embeddedCompiler = new AsyncEmbeddedCompiler(); @@ -197,7 +218,7 @@ async function compileRequestAsync( function compileRequestSync( request: proto.InboundMessage_CompileRequest, importers: ImporterRegistry<'sync'>, - options?: Options<'sync'> + options?: OptionsWithLegacy<'sync'> ): CompileResult { const functions = new FunctionRegistry(options?.functions); const embeddedCompiler = new SyncEmbeddedCompiler(); @@ -272,16 +293,23 @@ function createDispatcher( /** Handles a log event according to `options`. */ function handleLogEvent( - options: Options<'sync' | 'async'> | undefined, + options: OptionsWithLegacy<'sync' | 'async'> | undefined, event: proto.OutboundMessage_LogEvent ): void { + let span = event.span ? deprotofySourceSpan(event.span) : null; + if (span && options?.legacy) span = removeLegacyImporterFromSpan(span); + let message = event.message; + if (options?.legacy) message = removeLegacyImporter(message); + let formatted = event.formatted; + if (options?.legacy) formatted = removeLegacyImporter(formatted); + if (event.type === proto.LogEventType.DEBUG) { if (options?.logger?.debug) { - options.logger.debug(event.message, { - span: deprotofySourceSpan(event.span!), + options.logger.debug(message, { + span: span!, }); } else { - console.error(event.formatted); + console.error(formatted); } } else { if (options?.logger?.warn) { @@ -289,16 +317,16 @@ function handleLogEvent( { deprecation: event.type === proto.LogEventType.DEPRECATION_WARNING, }; - - const spanProto = event.span; - if (spanProto) params.span = deprotofySourceSpan(spanProto); + if (span) params.span = span; const stack = event.stackTrace; - if (stack) params.stack = stack; + if (stack) { + params.stack = options?.legacy ? removeLegacyImporter(stack) : stack; + } - options.logger.warn(event.message, params); + options.logger.warn(message, params); } else { - console.error(event.formatted); + console.error(formatted); } } } diff --git a/lib/src/legacy/importer.ts b/lib/src/legacy/importer.ts index 149c55cf..791cfc12 100644 --- a/lib/src/legacy/importer.ts +++ b/lib/src/legacy/importer.ts @@ -3,7 +3,6 @@ // https://opensource.org/licenses/MIT. import {strict as assert} from 'assert'; -import {pathToFileURL, URL as NodeURL} from 'url'; import * as fs from 'fs'; import * as p from 'path'; import * as util from 'util'; @@ -26,6 +25,12 @@ import { LegacyPluginThis, LegacySyncImporter, } from '../vendor/sass'; +import { + pathToLegacyFileUrl, + legacyFileUrlToPath, + legacyImporterProtocol, + legacyImporterProtocolPrefix, +} from './utils'; /** * A special URL protocol we use to signal when a stylesheet has finished @@ -36,9 +41,9 @@ import { export const endOfLoadProtocol = 'sass-embedded-legacy-load-done:'; /** - * The URL protocol to use for URLs canonicalized using `LegacyImporterWrapper`. + * The `file:` URL protocol with [legacyImporterProtocolPrefix] at the beginning. */ -export const legacyImporterProtocol = 'legacy-importer:'; +export const legacyImporterFileProtocol = 'legacy-importer-file:'; // A count of `endOfLoadProtocol` imports that have been generated. Each one // must be a different URL to ensure that the importer results aren't cached. @@ -68,11 +73,6 @@ export class LegacyImporterWrapper // `this.callbacks`, if it returned one. private lastContents: string | undefined; - // Whether we're expecting the next call to `canonicalize()` to be a relative - // load. The legacy importer API doesn't handle these loads in the same way as - // the modern API, so we always return `null` in this case. - private expectingRelativeLoad = true; - constructor( private readonly self: LegacyPluginThis, private readonly callbacks: Array>, @@ -90,14 +90,23 @@ export class LegacyImporterWrapper ): PromiseOr { if (url.startsWith(endOfLoadProtocol)) return new URL(url); - // Since there's only ever one modern importer in legacy mode, we can be - // sure that all normal loads are preceded by exactly one relative load. - if (this.expectingRelativeLoad) { - if (url.startsWith('file:')) { + if ( + url.startsWith(legacyImporterProtocolPrefix) || + url.startsWith(legacyImporterProtocol) + ) { + // A load starts with `legacyImporterProtocolPrefix` if and only if it's a + // relative load for the current importer rather than an absolute load. + // For the most part, we want to ignore these, but for `file:` URLs + // specifically we want to resolve them on the filesystem to ensure + // locality. + const urlWithoutPrefix = url.substring( + legacyImporterProtocolPrefix.length + ); + if (urlWithoutPrefix.startsWith('file:')) { let resolved: string | null = null; try { - const path = fileUrlToPathCrossPlatform(url); + const path = fileUrlToPathCrossPlatform(urlWithoutPrefix); resolved = resolvePath(path, options.fromImport); } catch (error: unknown) { if ( @@ -119,16 +128,11 @@ export class LegacyImporterWrapper if (resolved !== null) { this.prev.push({url: resolved, path: true}); - return pathToFileURL(resolved); + return pathToLegacyFileUrl(resolved); } } - this.expectingRelativeLoad = false; return null; - } else if (!url.startsWith('file:')) { - // We'll only search for another relative import when the URL isn't - // absolute. - this.expectingRelativeLoad = true; } const prev = this.prev[this.prev.length - 1]; @@ -153,14 +157,14 @@ export class LegacyImporterWrapper encodeURI((result as {file: string}).file) ); } else if (/^[A-Za-z+.-]+:/.test(url)) { - return new URL(url); + return new URL(`${legacyImporterProtocolPrefix}${url}`); } else { return new URL(legacyImporterProtocol + encodeURI(url)); } } else { if (p.isAbsolute(result.file)) { const resolved = resolvePath(result.file, options.fromImport); - return resolved ? pathToFileURL(resolved) : null; + return resolved ? pathToLegacyFileUrl(resolved) : null; } const prefixes = [...this.loadPaths, '.']; @@ -171,16 +175,16 @@ export class LegacyImporterWrapper p.join(prefix, result.file), options.fromImport ); - if (resolved !== null) return pathToFileURL(resolved); + if (resolved !== null) return pathToLegacyFileUrl(resolved); } return null; } }), result => { if (result !== null) { - const path = result.protocol === 'file:'; + const path = result.protocol === legacyImporterFileProtocol; this.prev.push({ - url: path ? fileUrlToPathCrossPlatform(result as NodeURL) : url, + url: path ? legacyFileUrlToPath(result) : url, path, }); return result; @@ -190,7 +194,7 @@ export class LegacyImporterWrapper p.join(loadPath, url), options.fromImport ); - if (resolved !== null) return pathToFileURL(resolved); + if (resolved !== null) return pathToLegacyFileUrl(resolved); } return null; } @@ -208,7 +212,7 @@ export class LegacyImporterWrapper }; } - if (canonicalUrl.protocol === 'file:') { + if (canonicalUrl.protocol === legacyImporterFileProtocol) { const syntax = canonicalUrl.pathname.endsWith('.sass') ? 'indented' : canonicalUrl.pathname.endsWith('.css') @@ -217,10 +221,7 @@ export class LegacyImporterWrapper let contents = this.lastContents ?? - fs.readFileSync( - fileUrlToPathCrossPlatform(canonicalUrl as NodeURL), - 'utf-8' - ); + fs.readFileSync(legacyFileUrlToPath(canonicalUrl), 'utf-8'); this.lastContents = undefined; if (syntax === 'scss') { contents += this.endOfLoadImport; @@ -311,7 +312,7 @@ export class LegacyImporterWrapper // The `@import` statement to inject after the contents of files to ensure // that we know when a load has completed so we can pass the correct `prev` // argument to callbacks. - private get endOfLoadImport() { + private get endOfLoadImport(): string { return `\n;@import "${endOfLoadProtocol}${endOfLoadCount++}";`; } } diff --git a/lib/src/legacy/index.ts b/lib/src/legacy/index.ts index 1631a534..8cc381e7 100644 --- a/lib/src/legacy/index.ts +++ b/lib/src/legacy/index.ts @@ -4,7 +4,7 @@ import * as fs from 'fs'; import * as p from 'path'; -import {URL, pathToFileURL} from 'url'; +import {pathToFileURL, URL} from 'url'; import {Exception} from '../exception'; import { @@ -33,11 +33,13 @@ import { StringOptions, } from '../vendor/sass'; import {wrapFunction} from './value/wrap'; +import {endOfLoadProtocol, LegacyImporterWrapper} from './importer'; import { - endOfLoadProtocol, legacyImporterProtocol, - LegacyImporterWrapper, -} from './importer'; + pathToLegacyFileUrl, + removeLegacyImporter, + removeLegacyImporterFromSpan, +} from './utils'; export function render( options: LegacyOptions<'async'>, @@ -74,8 +76,8 @@ export function renderSync(options: LegacyOptions<'sync'>): LegacyResult { } } -// Does some initial adjustments of `options` to make it easier to convert pass -// to the new API. +// Does some initial adjustments of `options` to make it easier to pass to the +// new API. function adjustOptions( options: LegacyOptions ): LegacyOptions { @@ -119,7 +121,7 @@ function isStringOptions( function convertOptions( options: LegacyOptions, sync: SyncBoolean -): Options { +): Options & {legacy: true} { if ( 'outputStyle' in options && options.outputStyle !== 'compressed' && @@ -165,6 +167,7 @@ function convertOptions( verbose: options.verbose, charset: options.charset, logger: options.logger, + legacy: true, }; } @@ -172,13 +175,15 @@ function convertOptions( function convertStringOptions( options: LegacyStringOptions, sync: SyncBoolean -): StringOptions { +): StringOptions & {legacy: true} { const modernOptions = convertOptions(options, sync); return { ...modernOptions, url: options.file - ? pathToFileURL(options.file) + ? options.importer + ? pathToLegacyFileUrl(options.file) + : pathToFileURL(options.file) : new URL(legacyImporterProtocol), importer: modernOptions.importers ? modernOptions.importers[0] : undefined, syntax: options.indentedSyntax ? 'indented' : 'scss', @@ -256,12 +261,11 @@ function newLegacyResult( sourceMap.sources = sourceMap.sources .filter(source => !source.startsWith(endOfLoadProtocol)) .map(source => { + source = removeLegacyImporter(source); if (source.startsWith('file://')) { return pathToUrlString( p.relative(sourceMapDir, fileUrlToPathCrossPlatform(source)) ); - } else if (source.startsWith(legacyImporterProtocol)) { - return source.substring(legacyImporterProtocol.length); } else if (source.startsWith('data:')) { return 'stdin'; } else { @@ -299,13 +303,14 @@ function newLegacyResult( includedFiles: result.loadedUrls .filter(url => url.protocol !== endOfLoadProtocol) .map(url => { - if (url.protocol === 'file:') { - return fileUrlToPathCrossPlatform(url as URL); - } else if (url.protocol === legacyImporterProtocol) { + if (url.protocol === legacyImporterProtocol) { return decodeURI(url.pathname); - } else { - return url.toString(); } + + const urlString = removeLegacyImporter(url.toString()); + return urlString.startsWith('file:') + ? fileUrlToPathCrossPlatform(urlString) + : urlString; }), }, }; @@ -321,25 +326,27 @@ function newLegacyException(error: Error): LegacyException { }); } + const span = error.span ? removeLegacyImporterFromSpan(error.span) : null; let file: string; - if (!error.span?.url) { + if (!span?.url) { file = 'stdin'; - } else if (error.span.url.protocol === 'file:') { + } else if (span.url.protocol === 'file:') { // We have to cast to Node's URL type here because the specified type is the // standard URL type which is slightly less featureful. `fileURLToPath()` // does work with standard URL objects in practice, but we know that we // generate Node URLs here regardless. - file = fileUrlToPathCrossPlatform(error.span.url as URL); + file = fileUrlToPathCrossPlatform(span.url as URL); } else { - file = error.span.url.toString(); + file = span.url.toString(); } + const errorString = removeLegacyImporter(error.toString()); return Object.assign(new Error(), { status: 1, - message: error.toString().replace(/^Error: /, ''), - formatted: error.toString(), - toString: () => error.toString(), - stack: error.stack, + message: errorString.replace(/^Error: /, ''), + formatted: errorString, + toString: () => errorString, + stack: error.stack ? removeLegacyImporter(error.stack) : undefined, line: isNullOrUndefined(error.span?.start.line) ? undefined : error.span!.start.line + 1, diff --git a/lib/src/legacy/utils.ts b/lib/src/legacy/utils.ts new file mode 100644 index 00000000..05ffe42b --- /dev/null +++ b/lib/src/legacy/utils.ts @@ -0,0 +1,59 @@ +// Copyright 2023 Google Inc. Use of this source code is governed by an +// MIT-style license that can be found in the LICENSE file or at +// https://opensource.org/licenses/MIT. + +import {strict as assert} from 'assert'; +import {pathToFileURL} from 'url'; + +import {fileUrlToPathCrossPlatform} from '../utils'; +import {SourceSpan} from '../vendor/sass'; +import {legacyImporterFileProtocol} from './importer'; + +/** + * The URL protocol to use for URLs canonicalized using `LegacyImporterWrapper`. + */ +export const legacyImporterProtocol = 'legacy-importer:'; + +/** + * The prefix for absolute URLs canonicalized using `LegacyImporterWrapper`. + * + * This is used to distinguish imports resolved relative to URLs returned by a + * legacy importer from manually-specified absolute URLs. + */ +export const legacyImporterProtocolPrefix = 'legacy-importer-'; + +/// A regular expression that matches legacy importer protocol syntax that +/// should be removed from human-readable messages. +const removeLegacyImporterRegExp = new RegExp( + `${legacyImporterProtocol}|${legacyImporterProtocolPrefix}`, + 'g' +); + +/// Returns `string` with all instances of legacy importer syntax removed. +export function removeLegacyImporter(string: string): string { + return string.replace(removeLegacyImporterRegExp, ''); +} + +/// Returns a copy of [span] with the URL updated to remove legacy importer +/// syntax. +export function removeLegacyImporterFromSpan(span: SourceSpan): SourceSpan { + if (!span.url) return span; + return {...span, url: new URL(removeLegacyImporter(span.url.toString()))}; +} + +/// Converts [path] to a `file:` URL and adds the [legacyImporterProtocolPrefix] +/// to the beginning so we can distinguish it from manually-specified absolute +/// `file:` URLs. +export function pathToLegacyFileUrl(path: string): URL { + return new URL(`${legacyImporterProtocolPrefix}${pathToFileURL(path)}`); +} + +/// Converts a `file:` URL with [legacyImporterProtocolPrefix] to the filesystem +/// path which it represents. +export function legacyFileUrlToPath(url: URL): string { + assert.equal(url.protocol, legacyImporterFileProtocol); + const originalUrl = url + .toString() + .substring(legacyImporterProtocolPrefix.length); + return fileUrlToPathCrossPlatform(originalUrl); +}