Skip to content

feat: SPA routing fallback support #491

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
lifeart wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: SPA routing fallback support #491

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
71e1f03
spa routing support
lifeart Sep 25, 2021
99b5a32
improve navigation detection and tests
lifeart Sep 27, 2021
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
33 changes: 29 additions & 4 deletions lib/middleware.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,16 @@ interface MiddlewareOptions {
liveReloadPath?: string;
}

function findClosestIndexFileForPath(outputPath: string, prefix: string): string | undefined {

@stefanpenner stefanpenner Sep 27, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Path manipulation from inbound requests always gives me the heebeegeebeez do to the potential of path traversal security issues. I would prefer to avoid as much liability here as possible. Do we think the fallback index.html lookup is something sufficiently commonly used to justify?

Or would a fixed index.html such as https://github.com/ember-cli/ember-cli/blob/2d77f099c19f2b54328e7e961e0b23a31a638661/lib/tasks/server/middleware/history-support/index.js#L63 be sufficient.

I can be convinced by either approach, the former will just require substantially more testing and care.

@lifeart lifeart Sep 27, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@stefanpenner we could skip index search if path contain . to prevent traversal security issues.

I seen cases where multiple static apps composed into one using nesting (and have to deal with it):

root_app
   /child-app
   /some-side-app
   /help-app

@stefanpenner stefanpenner Sep 29, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ya it’s not a bad feature at all.

@lifeart lifeart Sep 29, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@stefanpenner added guard for . in path 99b5a32#diff-92fc8ac710621cc64e5b3683c8c56bf5aec7a209c2968a1818bab04b11551811R87 and improved tests

const candidates = [];
const parts = prefix.split('/');
while (parts.length) {
parts.pop();
candidates.push(resolvePath(outputPath, [...parts, 'index.html'].join(path.sep)));
}
return candidates.find(file => fs.existsSync(file));
}

// You must call watcher.start() before you call `getMiddleware`
//
// This middleware is for development use only. It hasn't been reviewed
Expand All @@ -45,7 +55,7 @@ function handleRequest(
// eslint-disable-next-line node/no-deprecated-api
const urlObj = url.parse(request.url);
const pathname = urlObj.pathname || '';
let filename: string, stat;
let filename: string, stat!: fs.Stats;

try {
filename = decodeURIComponent(pathname);
Expand All @@ -66,9 +76,24 @@ function handleRequest(
try {
stat = fs.statSync(filename);
} catch (e) {
// not found
next();
return;
const nameStats = path.parse(filename);
Comment thread
lifeart marked this conversation as resolved.
const maybeIndex = findClosestIndexFileForPath(outputPath, filename.substr(1));

// if it's looks like an SPA path
if (nameStats.ext === '' && maybeIndex) {
filename = maybeIndex.replace(path.sep + 'index.html', '');
try {
stat = fs.statSync(filename);
} catch (e) {
// not found
Comment thread
lifeart marked this conversation as resolved.
Outdated
next();
return;
}
} else {
// not found
next();
return;
}
}

if (stat.isDirectory()) {
Expand Down
4 changes: 3 additions & 1 deletion test/builder_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import heimdall from 'heimdalljs';
const multidepRequire = MultidepRequire('test/multidep.json');
const Plugin = multidepRequire('broccoli-plugin', '1.3.0');
const broccoliSource = multidepRequire('broccoli-source', '1.1.0');
const isWin = os.platform() === 'win32';

const Builder = broccoli.Builder;
const expect = chai.expect;
Expand Down Expand Up @@ -1040,7 +1041,8 @@ describe('Builder', function() {
// the actual results of process.hrtime() are not
// reliable
if (process.env.CI !== 'true') {
expect(a).to.be.within(b, b + 10e6);
const delta = isWin ? 15e6 : 10e6;
expect(a).to.be.within(b, b + delta);
}
};

Expand Down
3 changes: 3 additions & 0 deletions test/fixtures/spa/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
<html>
<body>Hello from SPA</body>
</html>
31 changes: 31 additions & 0 deletions test/server_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,37 @@ describe('server', function() {
expect(statusCode).to.eql(200);
}).timeout(5000);

it('support SPA routing to index.html from child paths', async function() {
const mockUI = new MockUI();
const builder = new Builder(new broccoliSource.WatchedDir('test/fixtures/spa'));
const watcher = new Watcher(builder, []);
server = new Server.Server(watcher, '127.0.0.1', PORT, undefined, mockUI);
server.start();
await new Promise(resolve => {
server.instance.on('listening', resolve);
});
const { statusCode, body } = await got(`http://127.0.0.1:${PORT}/foo/bar/baz`); // basic serving
expect(statusCode).to.eql(200);
expect(body).to.contain('Hello from SPA');
}).timeout(5000);

it("skip SPA routing to index.html from child path if it's ends with extension", async function() {
const mockUI = new MockUI();
const builder = new Builder(new broccoliSource.WatchedDir('test/fixtures/spa'));
const watcher = new Watcher(builder, []);
server = new Server.Server(watcher, '127.0.0.1', PORT, undefined, mockUI);
server.start();
await new Promise(resolve => {
server.instance.on('listening', resolve);
});
try {
await got(`http://127.0.0.1:${PORT}/foo/bar/baz.png`);
expect.fail('expected rejection');
} catch (e) {
expect(e.body).to.include(`Cannot GET /foo/bar/baz.png`);
}
}).timeout(5000);

it('buildSuccess is handled', async function() {
const mockUI = new MockUI();
const builder = new Builder(new broccoliSource.WatchedDir('test/fixtures/basic'));
Expand Down