Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/better-results-press.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@rocket.chat/meteor": major
---

Removes `/ufs` legacy endpoint for downloading files
2 changes: 0 additions & 2 deletions apps/meteor/app/api/server/v1/rooms.ts
Original file line number Diff line number Diff line change
Expand Up @@ -307,8 +307,6 @@ API.v1.addRoute(
const fileStore = FileUpload.getStore('Uploads');
const uploadedFile = await fileStore.insert(details, file.tempFilePath);

uploadedFile.path = FileUpload.getPath(`${uploadedFile._id}/${encodeURI(uploadedFile.name || '')}`);

await Uploads.updateFileComplete(uploadedFile._id, this.userId, omit(uploadedFile, '_id'));

return API.v1.success({
Expand Down
16 changes: 14 additions & 2 deletions apps/meteor/app/apps/server/converters/uploads.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { Uploads } from '@rocket.chat/models';

import { transformMappedData } from './transformMappedData';
import { getURL } from '../../../utils/server/getURL';

export class AppUploadsConverter {
constructor(orch) {
Expand All @@ -19,6 +20,19 @@ export class AppUploadsConverter {
}

const map = {
// `url`/`path` are no longer persisted on the upload; derive them from the file id+name,
// matching the canonical /file-upload route. Declared before `id`/`name` so the source
// fields are still present on the cloned data when these run.
url: (upload) => {
const relativePath = `/file-upload/${upload._id}/${encodeURIComponent(upload.name || '')}`;
delete upload.url;
return getURL(relativePath, { cdn: false, full: true });
},
path: (upload) => {
const relativePath = `/file-upload/${upload._id}/${encodeURIComponent(upload.name || '')}`;
delete upload.path;
return relativePath;
Comment thread
KevLehman marked this conversation as resolved.
},
id: '_id',
name: 'name',
size: 'size',
Expand All @@ -30,9 +44,7 @@ export class AppUploadsConverter {
extension: 'extension',
progress: 'progress',
etag: 'etag',
path: 'path',
token: 'token',
url: 'url',
updatedAt: '_updatedAt',
uploadedAt: 'uploadedAt',
room: async (upload) => {
Expand Down
3 changes: 0 additions & 3 deletions apps/meteor/app/file-upload/server/config/GridFS.ts
Original file line number Diff line number Diff line change
Expand Up @@ -149,9 +149,6 @@ FileUpload.configureUploadsStore('GridFS', 'GridFS:UserDataFiles', {
collectionName: 'rocketchat_userDataFiles',
});

// DEPRECATED: backwards compatibility (remove)
UploadFS.getStores().rocketchat_uploads = UploadFS.getStores()['GridFS:Uploads'];

FileUpload.configureUploadsStore('GridFS', 'GridFS:Avatars', {
collectionName: 'rocketchat_avatars',
});
Expand Down
111 changes: 1 addition & 110 deletions apps/meteor/app/file-upload/server/lib/FileUpload.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ const { FileUpload, FileUploadClass } = proxyquire.noCallThru().load('./FileUplo
'../../../../server/ufs/ufs-methods': sinon.stub(),
'../../../settings/server': { settings: settingsStub },
'../../../utils/lib/mimeTypes': sinon.stub(),
'../../../utils/server/getURL': { getURL: sinon.stub() },
'../../../utils/server/lib/JWTHelper': {
validateAndDecodeJWT: validateAndDecodeJWTStub,
generateJWT: sinon.stub(),
Expand Down Expand Up @@ -358,114 +359,4 @@ describe('FileUpload', () => {
});
});
});

describe('getRequestUserId', () => {
it('should return undefined when no url is provided', async () => {
const request = { headers: {}, url: undefined } as any;

const result = await FileUpload.getRequestUserId(request);
expect(result).to.be.undefined;
expect(usersModelStub.findOneByIdAndLoginToken.called).to.be.false;
});

it('should return undefined when no credentials are provided', async () => {
const request = { headers: {}, url: '/ufs/UserDataFiles/file-id' } as any;

const result = await FileUpload.getRequestUserId(request);
expect(result).to.be.undefined;
expect(usersModelStub.findOneByIdAndLoginToken.called).to.be.false;
});

it('should return undefined when a uid is provided without a token', async () => {
const request = { headers: { 'x-user-id': 'user-1' }, url: '/ufs/UserDataFiles/file-id' } as any;

const result = await FileUpload.getRequestUserId(request);
expect(result).to.be.undefined;
expect(usersModelStub.findOneByIdAndLoginToken.called).to.be.false;
});

it('should return undefined when the login token is invalid', async () => {
usersModelStub.findOneByIdAndLoginToken.resolves(null);

const request = { headers: { 'x-user-id': 'user-1', 'x-auth-token': 'bad-token' }, url: '/ufs/UserDataFiles/file-id' } as any;

const result = await FileUpload.getRequestUserId(request);
expect(result).to.be.undefined;
expect(usersModelStub.findOneByIdAndLoginToken.calledOnceWith('user-1', 'hashed_bad-token')).to.be.true;
});

it('should return the user id when credentials are valid via headers', async () => {
usersModelStub.findOneByIdAndLoginToken.resolves({ _id: 'user-1' });

const request = { headers: { 'x-user-id': 'user-1', 'x-auth-token': 'good-token' }, url: '/ufs/UserDataFiles/file-id' } as any;

const result = await FileUpload.getRequestUserId(request);
expect(result).to.equal('user-1');
expect(usersModelStub.findOneByIdAndLoginToken.calledOnceWith('user-1', 'hashed_good-token')).to.be.true;
});

it('should return the user id when credentials are valid via query string', async () => {
usersModelStub.findOneByIdAndLoginToken.resolves({ _id: 'user-1' });

const request = { headers: {}, url: '/ufs/UserDataFiles/file-id?rc_uid=user-1&rc_token=good-token' } as any;

const result = await FileUpload.getRequestUserId(request);
expect(result).to.equal('user-1');
expect(usersModelStub.findOneByIdAndLoginToken.calledOnceWith('user-1', 'hashed_good-token')).to.be.true;
});
});

describe('UserDataFiles.onRead', () => {
// eslint-disable-next-line new-cap
const getOnRead = () => FileUpload.defaults.UserDataFiles().onRead;

const createResponse = () => {
const res = { writeHead: sinon.stub(), setHeader: sinon.stub() };
res.writeHead.returns(res);
return res as any;
};

it('should deny access to an unauthenticated request', async () => {
const res = createResponse();
const file = { _id: 'file-id', userId: 'owner-1', name: 'export.zip' } as any;
const request = { headers: {}, url: '/ufs/UserDataFiles/file-id' } as any;

const result = await getOnRead()('file-id', file, request, res);
expect(result).to.be.false;
expect(res.writeHead.calledOnceWith(403)).to.be.true;
expect(res.setHeader.called).to.be.false;
});

it('should deny access to an authenticated user who is not the owner', async () => {
usersModelStub.findOneByIdAndLoginToken.resolves({ _id: 'attacker-1' });

const res = createResponse();
const file = { _id: 'file-id', userId: 'owner-1', name: 'export.zip' } as any;
const request = {
headers: { 'x-user-id': 'attacker-1', 'x-auth-token': 'attacker-token' },
url: '/ufs/UserDataFiles/file-id',
} as any;

const result = await getOnRead()('file-id', file, request, res);
expect(result).to.be.false;
expect(res.writeHead.calledOnceWith(403)).to.be.true;
expect(res.setHeader.called).to.be.false;
});

it('should allow access to the owner of the export', async () => {
usersModelStub.findOneByIdAndLoginToken.resolves({ _id: 'owner-1' });

const res = createResponse();
const file = { _id: 'file-id', userId: 'owner-1', name: 'export.zip' } as any;
const request = {
headers: { 'x-user-id': 'owner-1', 'x-auth-token': 'owner-token' },
url: '/ufs/UserDataFiles/file-id',
} as any;

const result = await getOnRead()('file-id', file, request, res);
expect(result).to.be.true;
expect(res.writeHead.called).to.be.false;
expect(res.setHeader.calledOnceWith('content-disposition', 'attachment; filename="export.zip"')).to.be.true;
});
});
});
81 changes: 34 additions & 47 deletions apps/meteor/app/file-upload/server/lib/FileUpload.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import { MultipartUploadHandler } from '../../../api/server/lib/MultipartUploadH
import { canAccessRoomAsync, canAccessRoomIdAsync } from '../../../authorization/server/functions/canAccessRoom';
import { settings } from '../../../settings/server';
import { mime } from '../../../utils/lib/mimeTypes';
import { getURL } from '../../../utils/server/getURL';
import { validateAndDecodeJWT, generateJWT } from '../../../utils/server/lib/JWTHelper';
import { fileUploadIsValidContentType } from '../../../utils/server/restrictions';

Expand Down Expand Up @@ -62,16 +63,6 @@ const defaults: Record<string, () => Partial<StoreOptions>> = {
return `${settings.get('uniqueID')}/uploads/${file.rid}/${file.userId}/${file._id}`;
},
onValidate: FileUpload.uploadsOnValidate,
async onRead(_fileId: string, file: IUpload, req: http.IncomingMessage, res: http.ServerResponse) {
// Deprecated: Remove support to usf path
if (!(await FileUpload.requestCanAccessFiles(req, file))) {
res.writeHead(403);
return false;
}

res.setHeader('content-disposition', `attachment; filename="${encodeURIComponent(file.name || '')}"`);
return true;
},
};
},

Expand All @@ -97,17 +88,6 @@ const defaults: Record<string, () => Partial<StoreOptions>> = {
return `${settings.get('uniqueID')}/uploads/userData/${file.userId}/${file._id}`;
},
onValidate: FileUpload.uploadsOnValidate,
async onRead(_fileId: string, file: IUpload, req: http.IncomingMessage, res: http.ServerResponse) {
// UserDataFiles are GDPR data exports — only the owner of the export may download it.
const uid = await FileUpload.getRequestUserId(req);
if (!uid || uid !== file.userId) {
res.writeHead(403);
return false;
}

res.setHeader('content-disposition', `attachment; filename="${encodeURIComponent(file.name || '')}"`);
return true;
},
};
},
};
Expand Down Expand Up @@ -450,31 +430,6 @@ export const FileUpload = {
await Avatars.updateFileNameById(file._id, user.username);
},

async getRequestUserId({ headers = {}, url }: http.IncomingMessage): Promise<string | undefined> {
if (!url) {
return undefined;
}

const { query } = URL.parse(url, true);
// eslint-disable-next-line @typescript-eslint/naming-convention
let { rc_uid, rc_token } = query as Record<string, string | undefined>;

if (!rc_uid && headers.cookie) {
rc_uid = cookie.get('rc_uid', headers.cookie);
rc_token = cookie.get('rc_token', headers.cookie);
}

const uid = rc_uid || (headers['x-user-id'] as string);
const authToken = rc_token || (headers['x-auth-token'] as string);

if (!uid || !authToken) {
return undefined;
}

const user = await Users.findOneByIdAndLoginToken(uid, hashLoginToken(authToken), { projection: { _id: 1 } });
return user?._id;
},

async requestCanAccessFiles({ headers = {}, url }: http.IncomingMessage, file?: IUpload) {
if (!url || !settings.get('FileUpload_ProtectFiles')) {
return true;
Expand Down Expand Up @@ -908,12 +863,44 @@ export class FileUploadClass {
throw new Error('Invalid file type');
}

return ufsComplete(fileId, this.name, { session: options?.session });
const file = await ufsComplete(fileId, this.name, { session: options?.session });

// `/ufs` used to serve every store generically; it was replaced by per-store routes.
// Persist the store-aware public path/url so anything reading `IUpload.url`/`.path`
// (apps, integrations, …) keeps getting a link that resolves.
const path = this.getPublicPath(file);
if (path) {
const url = getURL(path, { cdn: false, full: true });
await this.model.updateOne({ _id: file._id }, { $set: { path, url } }, { session: options?.session });
file.path = path;
file.url = url;
}

return file;
} catch (e) {
throw e;
}
}

// The modern route depends on which store the file lives in (the old `/ufs` path served all of them).
private getPublicPath(file: IUpload): string | undefined {
if (this.model === Uploads) {
return FileUpload.getPath(`${file._id}/${encodeURIComponent(file.name || '')}`);
}
Comment thread
KevLehman marked this conversation as resolved.
if (this.model === UserDataFiles) {
return `/data-export/${file._id}`;
}
if (this.model === Avatars) {
if (file.rid) {
return `/avatar/room/${file.rid}`;
}
if (file.userId) {
return `/avatar/uid/${file.userId}`;
}
}
return undefined;
}

async insert(
fileData: Omit<OptionalId<IUpload>, '_updatedAt'>,
streamOrBuffer: stream.Readable | Buffer | string,
Expand Down
2 changes: 1 addition & 1 deletion apps/meteor/app/slackbridge/server/SlackAdapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1275,7 +1275,7 @@ export default class SlackAdapter {

const file = await fileStore.insert(details, stream);

const url = file.url.replace(Meteor.absoluteUrl(), '/');
const url = FileUpload.getPath(`${file._id}/${encodeURIComponent(file.name || '')}`);
const attachment = {
title: file.name,
title_link: url,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,11 @@ const FileItemMenu = ({ rid, fileData, onClickDelete }: FileItemMenuProps) => {
return;
}

if (fileData.url && fileData.name) {
if (fileData.name) {
const URL = window.webkitURL ?? window.URL;
const href = getURL(fileData.url);
const href = getURL(`/file-upload/${fileData._id}/${encodeURIComponent(fileData.name)}`);
download(href, fileData.name);
URL.revokeObjectURL(fileData.url);
URL.revokeObjectURL(href);
}
},
disabled: !canDownloadFile,
Expand Down
8 changes: 0 additions & 8 deletions apps/meteor/server/ufs/ufs-config.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
type ConfigOptions = {
https?: boolean;
simulateUploadSpeed?: number;
storesPath?: string;
tmpDir?: string;
tmpDirPermissions?: string;
};
Expand All @@ -13,8 +12,6 @@ export class Config {

public simulateUploadSpeed: RequiredConfigOptions['simulateUploadSpeed'];

public storesPath: RequiredConfigOptions['storesPath'];

public tmpDir: RequiredConfigOptions['tmpDir'];

public tmpDirPermissions: RequiredConfigOptions['tmpDirPermissions'];
Expand All @@ -24,7 +21,6 @@ export class Config {
options = {
https: false,
simulateUploadSpeed: 0,
storesPath: 'ufs',
tmpDir: '/tmp/ufs',
tmpDirPermissions: '0700',
...options,
Expand All @@ -37,9 +33,6 @@ export class Config {
if (typeof options.simulateUploadSpeed !== 'number') {
throw new TypeError('Config: simulateUploadSpeed is not a number');
}
if (typeof options.storesPath !== 'string') {
throw new TypeError('Config: storesPath is not a string');
}
if (typeof options.tmpDir !== 'string') {
throw new TypeError('Config: tmpDir is not a string');
}
Expand All @@ -49,7 +42,6 @@ export class Config {

this.https = options.https;
this.simulateUploadSpeed = options.simulateUploadSpeed;
this.storesPath = options.storesPath;
this.tmpDir = options.tmpDir;
this.tmpDirPermissions = options.tmpDirPermissions;
}
Expand Down
Loading
Loading