From df40b7bec4b753417283b8803bae801209848994 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Julian=20B=C3=A4ume?= <julian.baeume@open-xchange.com>
Date: Mon, 29 Jan 2024 18:32:57 +0100
Subject: [PATCH] Revert "Fix: OXUIB-2698: SSRF/XSS: Prevent to cache resources
from external origins"
This reverts commit d3bc298f8a95084af69a51f660c7b42b993ece43.
---
integration/caching_test.js | 9 ++-------
integration/config_test.js | 2 +-
integration/update-version_test.js | 4 ++--
spec/app_root_test.js | 2 +-
spec/file_caching_test.js | 7 +------
spec/redirect_test.js | 2 +-
spec/salt_test.js | 2 +-
spec/server_test.js | 4 ++--
spec/util.js | 7 +------
spec/version_mismatches_test.js | 2 +-
src/errors.js | 12 ------------
src/files.js | 8 ++------
src/routes/serve-files.js | 3 +--
13 files changed, 16 insertions(+), 48 deletions(-)
diff --git a/integration/caching_test.js b/integration/caching_test.js
index 92ef35b..4634526 100644
--- a/integration/caching_test.js
+++ b/integration/caching_test.js
@@ -30,7 +30,7 @@ describe('File caching service', function () {
let app, pubClient
beforeEach(async function () {
- await mockConfig({ baseUrls: ['http://ui-server'] })
+ await mockConfig({ baseUrls: ['http://ui-server/'] })
mockFetch({
'http://ui-server': {
'/manifest.json': generateSimpleViteManifest({
@@ -62,7 +62,7 @@ describe('File caching service', function () {
const version = response.headers.version
const { client } = await import('../src/redis.js')
- expect(await client.get(getRedisKey({ version, name: 'viteManifests' }))).to.equal('{"index.html":{"file":"index.html","meta":{"baseUrl":"http://ui-server"}}}')
+ expect(await client.get(getRedisKey({ version, name: 'viteManifests' }))).to.equal('{"index.html":{"file":"index.html","meta":{"baseUrl":"http://ui-server/"}}}')
const redisData = await client.getBuffer(getRedisKey({ version, name: 'oxManifests:body' }))
expect(zlib.brotliDecompressSync(redisData || '').toString()).to.equal('[]')
})
@@ -95,9 +95,4 @@ describe('File caching service', function () {
const response2 = await app.inject({ url: '/demo.js', headers: { version } })
expect(response2.statusCode).to.equal(200)
})
-
- it('does not fetch from origins not defined in baseUrls', async function () {
- const response = await app.inject({ url: '//t989be0.netlify.app/xss.html' })
- expect(response.statusCode).to.equal(400)
- })
})
diff --git a/integration/config_test.js b/integration/config_test.js
index 2a69ee8..b8f2c72 100644
--- a/integration/config_test.js
+++ b/integration/config_test.js
@@ -32,7 +32,7 @@ describe('Configuration', function () {
beforeEach(async function () {
// need to set the redis-prefix. Otherwise, the bull workers will interfere
process.env.REDIS_PREFIX = Math.random().toString()
- await mockConfig(config = { baseUrls: ['http://ui-server'] })
+ await mockConfig(config = { baseUrls: ['http://ui-server/'] })
mockFetch({
'http://ui-server': {
'/manifest.json': generateSimpleViteManifest({
diff --git a/integration/update-version_test.js b/integration/update-version_test.js
index d1673c9..3e154dc 100644
--- a/integration/update-version_test.js
+++ b/integration/update-version_test.js
@@ -33,7 +33,7 @@ describe('Updates the version', function () {
beforeEach(async function () {
// need to set the redis-prefix. Otherwise, the bull workers will interfere
process.env.REDIS_PREFIX = Math.random().toString()
- await mockConfig({ baseUrls: ['http://ui-server'] })
+ await mockConfig({ baseUrls: ['http://ui-server/'] })
mockFetch({
'http://ui-server': {
'/manifest.json': generateSimpleViteManifest({
@@ -109,7 +109,7 @@ describe('Updates the version', function () {
td.reset()
// need to set the redis-prefix. Otherwise, the bull workers will interfere
process.env.REDIS_PREFIX = Math.random().toString()
- await mockConfig({ baseUrls: ['http://ui-server'] })
+ await mockConfig({ baseUrls: ['http://ui-server/'] })
mockFetch({
'http://ui-server': {
'/manifest.json': generateSimpleViteManifest({
diff --git a/spec/app_root_test.js b/spec/app_root_test.js
index ddb3d8b..9de7bc0 100644
--- a/spec/app_root_test.js
+++ b/spec/app_root_test.js
@@ -29,7 +29,7 @@ describe('With different app root', function () {
let app
beforeEach(async function () {
- await mockConfig({ baseUrls: ['http://ui-server'] })
+ await mockConfig({ baseUrls: ['http://ui-server/'] })
const { client } = await mockRedis()
mockFetch({
'http://ui-server': {
diff --git a/spec/file_caching_test.js b/spec/file_caching_test.js
index ff74708..e9e68e9 100644
--- a/spec/file_caching_test.js
+++ b/spec/file_caching_test.js
@@ -37,7 +37,7 @@ describe('File caching service', function () {
let redis
beforeEach(async function () {
- await mockConfig({ baseUrls: ['http://ui-server'] })
+ await mockConfig({ baseUrls: ['http://ui-server/'] })
redis = await mockRedis()
mockFetch({
'http://ui-server': {}
@@ -273,9 +273,4 @@ describe('File caching service', function () {
// check for files in redis
expect(await redis.client.getBuffer('ui-middleware:554855300:/file.svg:body')).to.deep.equal(response.rawPayload)
})
-
- it('does not fetch from origins not defined in baseUrls', async function () {
- const response = await app.inject({ url: '//t989be0.netlify.app/xss.html' })
- expect(response.statusCode).to.equal(400)
- })
})
diff --git a/spec/redirect_test.js b/spec/redirect_test.js
index 03556d7..04cbefc 100644
--- a/spec/redirect_test.js
+++ b/spec/redirect_test.js
@@ -29,7 +29,7 @@ describe('Redirects', function () {
let app
before(async function () {
- await mockConfig({ baseUrls: ['http://ui-server'] })
+ await mockConfig({ baseUrls: ['http://ui-server/'] })
await mockRedis()
mockFetch({
'http://ui-server': {
diff --git a/spec/salt_test.js b/spec/salt_test.js
index 28a4621..bff60e3 100644
--- a/spec/salt_test.js
+++ b/spec/salt_test.js
@@ -30,7 +30,7 @@ describe('Salt', function () {
let config
beforeEach(async function () {
- await mockConfig(config = { baseUrls: ['http://ui-server'] })
+ await mockConfig(config = { baseUrls: ['http://ui-server/'] })
await mockRedis()
mockFetch({
'http://ui-server': {
diff --git a/spec/server_test.js b/spec/server_test.js
index b539722..fe50fca 100644
--- a/spec/server_test.js
+++ b/spec/server_test.js
@@ -30,7 +30,7 @@ describe('UI Middleware', function () {
let fetchConfig
beforeEach(async function () {
- await mockConfig({ baseUrls: ['http://ui-server'] })
+ await mockConfig({ baseUrls: ['http://ui-server/'] })
await mockRedis()
mockFetch(fetchConfig = {
'http://ui-server': {
@@ -77,7 +77,7 @@ describe('UI Middleware', function () {
describe('multiple configurations', function () {
beforeEach(async function () {
- await mockConfig({ baseUrls: ['http://ui-server', 'http://ui-server2'] })
+ await mockConfig({ baseUrls: ['http://ui-server/', 'http://ui-server2/'] })
fetchConfig['http://ui-server2'] = {
'/manifest.json': generateSimpleViteManifest({ 'example2.js': 'thing' }),
'/example2.js': ''
diff --git a/spec/util.js b/spec/util.js
index 964b1da..1e7a951 100644
--- a/spec/util.js
+++ b/spec/util.js
@@ -60,12 +60,7 @@ export function mockConfig (obj = {}) {
export function mockFetch (servers = {}) {
td.replace(global, 'fetch', async function ({ origin, pathname }, ...args) {
const response = servers[origin]?.[pathname]
- if (response === undefined) {
- if (origin === 'http://t989be0.netlify.app') {
- return new Response('<html><code>Proof of Concept</code><script>alert(document.domain)</script></html>', { status: 200, headers: { 'Content-Type': 'text/html' } })
- }
- return new Response('', { status: 404 })
- }
+ if (response === undefined) return new Response('', { status: 404 })
if (response instanceof Function) return response.apply(this, arguments)
if (typeof response === 'object') {
diff --git a/spec/version_mismatches_test.js b/spec/version_mismatches_test.js
index 9502632..278d505 100644
--- a/spec/version_mismatches_test.js
+++ b/spec/version_mismatches_test.js
@@ -32,7 +32,7 @@ describe('version mismatches', function () {
let runUpdate
beforeEach(async function () {
- await mockConfig({ baseUrls: ['http://ui-server'] })
+ await mockConfig({ baseUrls: ['http://ui-server/'] })
const { createClient } = await mockRedis()
mockFetch({
'http://ui-server': {
diff --git a/src/errors.js b/src/errors.js
index 45ff12f..5b63080 100644
--- a/src/errors.js
+++ b/src/errors.js
@@ -27,13 +27,6 @@ export class NotFoundError extends Error {
}
}
-export class NotAllowedOriginError extends Error {
- constructor (message, options = {}) {
- super(message, options)
- this.status = options.status
- }
-}
-
export class VersionMismatchError extends Error {}
/**
@@ -55,8 +48,3 @@ export function isNotFoundError (err) {
const errors = err instanceof AggregateError ? err.errors : [err]
return errors.some(error => error instanceof NotFoundError)
}
-
-export function isNotAllowedOriginError (err) {
- const errors = err instanceof AggregateError ? err.errors : [err]
- return errors.some(error => error instanceof NotAllowedOriginError)
-}
diff --git a/src/files.js b/src/files.js
index e0f749f..41e47ee 100644
--- a/src/files.js
+++ b/src/files.js
@@ -25,7 +25,7 @@ import { promisify } from 'node:util'
import zlib from 'node:zlib'
import * as cache from './cache.js'
import { configMap } from './config_map.js'
-import { NotAllowedOriginError, NotFoundError, VersionMismatchError, isVersionMismatchError } from './errors.js'
+import { NotFoundError, VersionMismatchError, isVersionMismatchError } from './errors.js'
import logger from './logger.js'
import { getCSSDependenciesFor, getViteManifests } from './manifests.js'
import { getVersionInfo } from './version.js'
@@ -38,12 +38,8 @@ const compressionMimeTypes = (process.env.COMPRESS_FILE_TYPES || '').replace(/([
const compressionWhitelistRegex = new RegExp(`^(${compressionMimeTypes.join('|')})($|;)`, 'i')
export async function fetchFileWithHeadersFromBaseUrl ({ path, baseUrl, version }) {
- const upstreamUrl = new URL(path, baseUrl)
- if (upstreamUrl.origin !== baseUrl) {
- throw new NotAllowedOriginError('This origin is not allowed', { status: 400 })
- }
const [response, dependencies] = await Promise.all([
- fetch(upstreamUrl, { cache: 'no-store' }),
+ fetch(new URL(path, baseUrl), { cache: 'no-store' }),
nodePath.extname(path) === '.js' && getCSSDependenciesFor({ file: path.substr(1), version })
])
diff --git a/src/routes/serve-files.js b/src/routes/serve-files.js
index 12f9544..fd8cfb7 100644
--- a/src/routes/serve-files.js
+++ b/src/routes/serve-files.js
@@ -21,7 +21,7 @@
*/
import { getFile } from '../files.js'
-import { isNotAllowedOriginError, isNotFoundError, isVersionMismatchError } from '../errors.js'
+import { isNotFoundError, isVersionMismatchError } from '../errors.js'
export default async function serveFilePlugin (fastify, options) {
fastify.get('*', async (req, reply) => {
@@ -35,7 +35,6 @@ export default async function serveFilePlugin (fastify, options) {
reply.send(body)
} catch (err) {
if (isNotFoundError(err) || isVersionMismatchError(err)) throw fastify.httpErrors.createError(404, `File "${req.urlData('path')}" does not exist.`, err)
- if (isNotAllowedOriginError(err)) throw fastify.httpErrors.createError(400, err)
throw fastify.httpErrors.createError(err.statusCode || 500, err)
}
})
--
GitLab