Skip to content

A file is local if it exists in the root folder or a subfolder #72

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

Merged
merged 6 commits into from
Nov 2, 2023
Merged

A file is local if it exists in the root folder or a subfolder #72

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
da08d8e
A file is local if it exists in the root folder or a subfolder. Work …
Fil Oct 26, 2023
3171f4d
Merge branch 'main' into fil/maybeLocalFile
Fil Oct 31, 2023
5f6638f
Merge branch 'main' into fil/maybeLocalFile
Fil Nov 1, 2023
2d52808
Merge branch 'fil/maybeLocalFile' of github.com:observablehq/cli into…
Fil Nov 1, 2023
4c59e17
apply suggestions from review
Fil Nov 1, 2023
2505146
don't path-test refs that start with https://
Fil Nov 1, 2023
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
16 changes: 15 additions & 1 deletion src/files.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,21 @@ import {readdir, stat} from "node:fs/promises";
import {extname, join, normalize, relative} from "node:path";
import {isNodeError} from "./error.js";

export function canReadSync(path: string): boolean {
// A file is local if it exists in the root folder or a subfolder.
export function isLocalFile(ref: string | null, root: string): boolean {
return (
typeof ref === "string" &&
!/^(\w+:)\/\//.test(ref) &&
!normalize(ref).startsWith("../") &&
canReadSync(join(root, ref))
);
}

export function pathFromRoot(ref: string | null, root: string): string | null {
return isLocalFile(ref, root) ? join(root, ref!) : null;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

thoughts on splitting this function into 2 - one for checking isLocalFile and the other for returning the pathFromRoot string? I was thinking

// A file is local if it exists in the root folder or a subfolder.
function isLocalFile(ref: string | null, root: string): boolean {
	if (typeof ref !== "string || !canReadSync(join(root, ref))) return false;
	return true;
}

function pathFromRoot(ref: string | null, root: string): string {
	if (isLocalFile(ref, root))) return join(root, ref);
	throw Error(`${ref} does not exist within the ${root} root directory`)
}

it's a little verbose but we can just use isLocalFile in src/javascript.ts

join should already normalize the paths (according to the docs)

Copy link
Contributor

Choose a reason for hiding this comment

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

instead of throwing an error we can just return null for pathFromRoot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks. I've applied your suggestions, still making sure that the normalized ref itself does not start with ../, otherwise it would allow to reach ../public/database.js for example.


function canReadSync(path: string): boolean {
try {
accessSync(path, constants.R_OK);
return statSync(path).isFile();
Expand Down
5 changes: 2 additions & 3 deletions src/javascript.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import {Parser, tokTypes, type Options} from "acorn";
import mime from "mime";
import {join} from "node:path";
import {canReadSync} from "./files.js";
import {isLocalFile} from "./files.js";
import {findAwaits} from "./javascript/awaits.js";
import {findDeclarations} from "./javascript/declarations.js";
import {findFeatures} from "./javascript/features.js";
Expand Down Expand Up @@ -50,7 +49,7 @@ export function transpileJavaScript(input: string, options: ParseOptions): Trans
const databases = node.features.filter((f) => f.type === "DatabaseClient").map((f) => ({name: f.name}));
const files = node.features
.filter((f) => f.type === "FileAttachment")
.filter((f) => canReadSync(join(root, f.name)))
.filter((f) => isLocalFile(f.name, root))
.map((f) => ({name: f.name, mimeType: mime.getType(f.name)}));
const inputs = Array.from(new Set<string>(node.references.map((r) => r.name)));
const output = new Sourcemap(input);
Expand Down
8 changes: 3 additions & 5 deletions src/markdown.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@ import {type RuleInline} from "markdown-it/lib/parser_inline.js";
import {type default as Renderer, type RenderRule} from "markdown-it/lib/renderer.js";
import mime from "mime";
import {readFile} from "node:fs/promises";
import {join} from "node:path";
import {canReadSync} from "./files.js";
import {pathFromRoot} from "./files.js";
import {computeHash} from "./hash.js";
import {transpileJavaScript, type FileReference, type ImportReference, type Transpile} from "./javascript.js";

Expand Down Expand Up @@ -301,9 +300,8 @@ function renderIntoPieces(renderer: Renderer, root: string): Renderer["render"]
function normalizePieceHtml(html: string, root: string, context: ParseContext): string {
const {document} = parseHTML(html);
for (const element of document.querySelectorAll("link[href]") as any as Iterable<Element>) {
const href = element.getAttribute("href")!;
if (/^(\w+:)\/\//.test(href)) continue; // absolute url
if (canReadSync(join(root, href))) {
const href = pathFromRoot(element.getAttribute("href"), root);
if (href) {
context.files.push({name: href, mimeType: mime.getType(href)});
element.setAttribute("href", `/_file/${href}`);
}
Expand Down