Skip to content

Commit

Permalink
Avoid relying on buggy import order in the legacy importer (#245)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@nex3
nex3 committed Sep 8, 2023
1 parent 489abd5 commit 6129adf
Show file tree
Hide file tree
Showing 4 changed files with 167 additions and 72 deletions.
62 changes: 45 additions & 17 deletions lib/src/compile.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<sync extends 'sync' | 'async'> = Options<sync> & {
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<sync extends 'sync' | 'async'> =
StringOptions<sync> & {legacy?: boolean};

export function compile(
path: string,
options?: Options<'sync'>
options?: OptionsWithLegacy<'sync'>
): CompileResult {
const importers = new ImporterRegistry(options);
return compileRequestSync(
Expand All @@ -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(
Expand All @@ -46,7 +67,7 @@ export function compileString(

export function compileAsync(
path: string,
options?: Options<'async'>
options?: OptionsWithLegacy<'async'>
): Promise<CompileResult> {
const importers = new ImporterRegistry(options);
return compileRequestAsync(
Expand All @@ -58,7 +79,7 @@ export function compileAsync(

export function compileStringAsync(
source: string,
options?: StringOptions<'async'>
options?: StringOptionsWithLegacy<'async'>
): Promise<CompileResult> {
const importers = new ImporterRegistry(options);
return compileRequestAsync(
Expand Down Expand Up @@ -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<CompileResult> {
const functions = new FunctionRegistry(options?.functions);
const embeddedCompiler = new AsyncEmbeddedCompiler();
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -272,33 +293,40 @@ function createDispatcher<sync extends 'sync' | 'async'>(

/** 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) {
const params: {deprecation: boolean; span?: SourceSpan; stack?: string} =
{
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);
}
}
}
Expand Down
63 changes: 32 additions & 31 deletions lib/src/legacy/importer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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
Expand All @@ -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.
Expand Down Expand Up @@ -68,11 +73,6 @@ export class LegacyImporterWrapper<sync extends 'sync' | 'async'>
// `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<LegacyImporter<sync>>,
Expand All @@ -90,14 +90,23 @@ export class LegacyImporterWrapper<sync extends 'sync' | 'async'>
): PromiseOr<URL | null, sync> {
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 (
Expand All @@ -119,16 +128,11 @@ export class LegacyImporterWrapper<sync extends 'sync' | 'async'>

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];
Expand All @@ -153,14 +157,14 @@ export class LegacyImporterWrapper<sync extends 'sync' | 'async'>
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, '.'];
Expand All @@ -171,16 +175,16 @@ export class LegacyImporterWrapper<sync extends 'sync' | 'async'>
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;
Expand All @@ -190,7 +194,7 @@ export class LegacyImporterWrapper<sync extends 'sync' | 'async'>
p.join(loadPath, url),
options.fromImport
);
if (resolved !== null) return pathToFileURL(resolved);
if (resolved !== null) return pathToLegacyFileUrl(resolved);
}
return null;
}
Expand All @@ -208,7 +212,7 @@ export class LegacyImporterWrapper<sync extends 'sync' | 'async'>
};
}

if (canonicalUrl.protocol === 'file:') {
if (canonicalUrl.protocol === legacyImporterFileProtocol) {
const syntax = canonicalUrl.pathname.endsWith('.sass')
? 'indented'
: canonicalUrl.pathname.endsWith('.css')
Expand All @@ -217,10 +221,7 @@ export class LegacyImporterWrapper<sync extends 'sync' | 'async'>

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;
Expand Down Expand Up @@ -311,7 +312,7 @@ export class LegacyImporterWrapper<sync extends 'sync' | 'async'>
// 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++}";`;
}
}
Loading

0 comments on commit 6129adf

Please sign in to comment.