Skip to content

Commit

Permalink
fix: Allow JPEG and GIF images as profile photos #2332 (#2353)
Browse files Browse the repository at this point in the history
* Fixes UserProfileStore and UserProfileContextMenu to allow JPEG and GIF profile photos as well as PNG.
* Updates E2E tests to include JPEG and GIF cases on profile photo tests
* Adds new backend unit tests for img validation
* Adds new npm scripts at the root for conveniently running the desktop app locally instead of having to cd into the desktop package folder and linting all packages at once.
* Adds new npm scripts for local binary compilation and updates E2E tests allow using local binaries
* Adds documentation for new local binary compilation and testing scripts
* Fixes issues I had while running tor tests locally
* Fix the issue with ContextMenuProps having type unknown for the field visible
  • Loading branch information
Isla Koenigsknecht authored Mar 14, 2024
1 parent 3f03d32 commit 233725f
Show file tree
Hide file tree
Showing 32 changed files with 504 additions and 179 deletions.
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@ c4/workspace.json
.ignore
.DS_Store
.vscode
packages/.DS_Store
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@

* Use ack for CREATE_NETWORK and simplify

# Fixes

* Allow JPEG and GIF files as profile photos ([#2332](https://github.com/TryQuiet/quiet/issues/2332))

[2.1.2]

# Refactorings:
Expand Down
5 changes: 4 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@
"scripts": {
"lerna": "lerna",
"publish": "lerna version $npm_config_release --no-private",
"postpublish": "node copy-changelog.js && git add . && git commit -m 'Update packages CHANGELOG.md' && git push"
"postpublish": "node copy-changelog.js && git add . && git commit -m 'Update packages CHANGELOG.md' && git push",
"start:desktop": "lerna run --scope @quiet/desktop start",
"lint:all": "lerna run lint",
"distAndRunE2ETests:mac:local": "lerna run --scope @quiet/desktop distMac:local && lerna run --scope e2e-tests test:localBinary --"
},
"engines": {
"node": "18.12.1",
Expand Down
3 changes: 2 additions & 1 deletion packages/backend/.gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,5 @@
/coverage
/testingFixtures/certificates/files
/testingFixtures/certificates/files2
/src/storage/testUtils/large-file*
/src/nest/storage/testUtils/large-file*
/src/ipfs-file-manager/testUtils/large-file*
4 changes: 4 additions & 0 deletions packages/backend/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@

* Feature: add functionality to export chat to text document in desktop version

* Fix: Updates UserProfileStore to allow JPEG and GIF files ([#2332](https://github.com/TryQuiet/quiet/issues/2332))

* Refactor: Consolidate profile photo validation and match magic byte check to type check

[2.0.3-alpha.6]

* Fix: filter out invalid peer addresses in peer list. Update peer list in localdb.
Expand Down
2 changes: 1 addition & 1 deletion packages/backend/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
"lint": "eslint --ext .jsx,.js,.ts,.tsx ./src/ --fix",
"lint-ci": "eslint --ext .jsx,.js,.ts,.tsx ./src/",
"test-nest": "cross-env NODE_OPTIONS=--experimental-vm-modules DEBUG=ipfs:*,backend:* node_modules/jest/bin/jest.js --detectOpenHandles --forceExit ./src/nest/**/*.spec.ts",
"test": "cross-env NODE_OPTIONS=--experimental-vm-modules DEBUG=ipfs:*,backend:* node_modules/jest/bin/jest.js ./src/**/* --runInBand --verbose --testPathIgnorePatterns=\".src/(!?nodeTest*)|(.node_modules*)\"",
"test": "cross-env NODE_OPTIONS=--experimental-vm-modules DEBUG=ipfs:*,backend:* jest --runInBand --verbose --testPathIgnorePatterns=\".src/(!?nodeTest*)|(.node_modules*)\" --",
"test-ci": "cross-env NODE_OPTIONS=--experimental-vm-modules jest ./src/**/* --runInBand --colors --ci --silent --verbose --testPathIgnorePatterns=\".src/nest/(!?nodeTest*)|(.node_modules*)|src/nest/.*\\.tor.spec\\.(t|j)s|src/nest/ipfs-file-manager/big-files.long.spec.ts$\"",
"test-ci-tor": "cross-env NODE_OPTIONS=--experimental-vm-modules jest --runInBand --colors --ci --silent --verbose --detectOpenHandles --forceExit ./src/nest/**/*.tor.spec.ts",
"test-ci-long-running": "cross-env DEBUG=backend:* NODE_OPTIONS=--experimental-vm-modules jest --colors --ci --verbose ./src/nest/**/*.long.spec.ts",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,59 +9,7 @@ import { getCrypto, PublicKeyInfo } from 'pkijs'
import { ChannelMessage, NoCryptoEngineError, PublicChannel, UserProfile } from '@quiet/types'
import { configCrypto, generateKeyPair, sign } from '@quiet/identity'

import { isPng, base64DataURLToByteArray, UserProfileStore, UserProfileKeyValueIndex } from './userProfile.store'

describe('UserProfileStore/isPng', () => {
test('returns true for a valid PNG', () => {
// Bytes in decimal copied out of a PNG file
// e.g. od -t u1 ~/Pictures/test.png | less
const png = new Uint8Array([137, 80, 78, 71, 13, 10, 26, 10, 0, 0, 0, 13, 73, 72, 68, 82])
expect(isPng(png)).toBeTruthy()
})

test('returns false for a invalid PNG', () => {
// Changed the first byte from 137 to 136
const png = new Uint8Array([136, 80, 78, 71, 13, 10, 26, 10, 0, 0, 0, 13, 73, 72, 68, 82])
expect(isPng(png)).toBeFalsy()
})

test('returns false for a incomplete PNG', () => {
// Removed last byte from the PNG header
const png = new Uint8Array([137, 80, 78, 71, 13, 10, 26])
expect(isPng(png)).toBeFalsy()
})
})

describe('UserProfileStore/base64DataURLToByteArray', () => {
test("throws error if data URL prefix doesn't exist", () => {
const contents = '1234567'
expect(() => base64DataURLToByteArray(contents)).toThrow(Error)
})

test('throws error if data URL prefix is malformatted', () => {
let contents = ',1234567'
expect(() => base64DataURLToByteArray(contents)).toThrow(Error)

contents = ',1234567'
expect(() => base64DataURLToByteArray(contents)).toThrow(Error)

contents = 'data:,1234567'
expect(() => base64DataURLToByteArray(contents)).toThrow(Error)

contents = ';base64,1234567'
expect(() => base64DataURLToByteArray(contents)).toThrow(Error)

contents = 'dat:;base64,1234567'
expect(() => base64DataURLToByteArray(contents)).toThrow(Error)
})

test('returns Uint8Array if data URL prefix is correct', () => {
// base64 encoding of binary 'm'
// btoa('m') == 'bQ=='
const contents = 'data:mime;base64,bQ=='
expect(base64DataURLToByteArray(contents)).toEqual(new Uint8Array(['m'.charCodeAt(0)]))
})
})
import { UserProfileStore, UserProfileKeyValueIndex } from './userProfile.store'

const getUserProfile = async ({
pngByteArray,
Expand Down
67 changes: 2 additions & 65 deletions packages/backend/src/nest/storage/userProfile/userProfile.store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,47 +17,10 @@ import createLogger from '../../common/logger'
import { OrbitDb } from '../orbitDb/orbitDb.service'
import { StorageEvents } from '../storage.types'
import { KeyValueIndex } from '../orbitDb/keyValueIndex'
import { validatePhoto } from './userProfile.utils'

const logger = createLogger('UserProfileStore')

/**
* Check magic byte sequence to determine if buffer is a PNG image.
*/
export const isPng = (buffer: Uint8Array): boolean => {
// https://en.wikipedia.org/wiki/PNG
const pngHeader = [0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a]

if (buffer.length < pngHeader.length) {
return false
}

for (let i = 0; i < pngHeader.length; i++) {
if (buffer[i] !== pngHeader[i]) {
return false
}
}
return true
}

/**
* Takes a base64 data URI string that starts with 'data:*\/*;base64,'
* as returned from
* https://developer.mozilla.org/en-US/docs/Web/API/FileReader/readAsDataURL
* and converts it to a Uint8Array.
*/
export const base64DataURLToByteArray = (contents: string): Uint8Array => {
const [header, base64Data] = contents.split(',')
if (!header.startsWith('data:') || !header.endsWith(';base64')) {
throw new Error('Expected base64 data URI')
}
const chars = atob(base64Data)
const bytes = new Array(chars.length)
for (let i = 0; i < chars.length; i++) {
bytes[i] = chars.charCodeAt(i)
}
return new Uint8Array(bytes)
}

@Injectable()
export class UserProfileStore extends EventEmitter {
public store: KeyValueStore<UserProfile>
Expand Down Expand Up @@ -165,33 +128,7 @@ export class UserProfileStore extends EventEmitter {
return false
}

if (typeof profile.photo !== 'string') {
logger.error('Expected PNG for user profile photo', userProfile.pubKey)
return false
}

if (!profile.photo.startsWith('data:image/png;base64,')) {
logger.error('Expected PNG for user profile photo', userProfile.pubKey)
return false
}

// We only accept PNG for now. I think some care needs to be used
// with the Image element since it can make web requests and
// accepts a variety of formats that we may want to limit. Some
// interesting thoughts:
// https://security.stackexchange.com/a/135636
const photoBytes = base64DataURLToByteArray(profile.photo)
if (!isPng(photoBytes)) {
logger.error('Expected PNG for user profile photo', userProfile.pubKey)
return false
}

// 200 KB = 204800 B limit
//
// TODO: Perhaps the compression matters and we should check
// actual dimensions in pixels?
if (photoBytes.length > 204800) {
logger.error('User profile photo must be less than or equal to 200KB')
if (!validatePhoto(profile.photo, userProfile.pubKey)) {
return false
}
} catch (err) {
Expand Down

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
import { describe, expect, test } from '@jest/globals'

import { isPng, base64DataURLToByteArray, isGif, isJpeg, validatePhoto } from './userProfile.utils'
import { LARGE_IMG_URI, VALID_GIF_URI, VALID_JPEG_URI, VALID_PNG_URI } from './userProfile.utils.spec.const'

describe('isPng', () => {
test('returns true for a valid PNG', () => {
// Bytes in decimal copied out of a PNG file
// e.g. od -t u1 ~/Pictures/test.png | less
const png = new Uint8Array([137, 80, 78, 71, 13, 10, 26, 10, 0, 0, 0, 13, 73, 72, 68, 82])
expect(isPng(png)).toBeTruthy()
})

test('returns false for a invalid PNG', () => {
// Changed the first byte from 137 to 136
const png = new Uint8Array([136, 80, 78, 71, 13, 10, 26, 10, 0, 0, 0, 13, 73, 72, 68, 82])
expect(isPng(png)).toBeFalsy()
})

test('returns false for a incomplete PNG', () => {
// Removed last byte from the PNG header
const png = new Uint8Array([137, 80, 78, 71, 13, 10, 26])
expect(isPng(png)).toBeFalsy()
})
})

describe('isJpeg', () => {
test('returns true for a valid JPEG', () => {
const jpeg = new Uint8Array([255, 216, 255, 71, 13, 10, 26, 10, 0, 0, 0, 13, 73, 72, 68, 82])
expect(isJpeg(jpeg)).toBeTruthy()
})

test('returns false for a invalid JPEG', () => {
const jpeg = new Uint8Array([136, 80, 78, 71, 13, 10, 26, 10, 0, 0, 0, 13, 73, 72, 68, 82])
expect(isJpeg(jpeg)).toBeFalsy()
})

test('returns false for a incomplete JPEG', () => {
// Removed last byte from the PNG header
const jpeg = new Uint8Array([255, 216])
expect(isJpeg(jpeg)).toBeFalsy()
})
})

describe('isGif', () => {
test('returns true for a valid GIF89', () => {
const gif = new Uint8Array([71, 73, 70, 56, 57, 97, 26, 10, 0, 0, 0, 13, 73, 72, 68, 82])
expect(isGif(gif)).toBeTruthy()
})

test('returns true for a valid GIF87', () => {
const gif = new Uint8Array([71, 73, 70, 56, 55, 97, 26, 10, 0, 0, 0, 13, 73, 72, 68, 82])
expect(isGif(gif)).toBeTruthy()
})

test('returns false for a invalid GIF', () => {
const gif = new Uint8Array([136, 80, 78, 71, 13, 10, 26, 10, 0, 0, 0, 13, 73, 72, 68, 82])
expect(isGif(gif)).toBeFalsy()
})

test('returns false for a incomplete GIF', () => {
// Removed last byte from the PNG header
const gif = new Uint8Array([71, 73, 70, 56, 57])
expect(isGif(gif)).toBeFalsy()
})
})

describe('base64DataURLToByteArray', () => {
test("throws error if data URL prefix doesn't exist", () => {
const contents = '1234567'
expect(() => base64DataURLToByteArray(contents)).toThrow(Error)
})

test('throws error if data URL prefix is malformatted', () => {
let contents = ',1234567'
expect(() => base64DataURLToByteArray(contents)).toThrow(Error)

contents = ',1234567'
expect(() => base64DataURLToByteArray(contents)).toThrow(Error)

contents = 'data:,1234567'
expect(() => base64DataURLToByteArray(contents)).toThrow(Error)

contents = ';base64,1234567'
expect(() => base64DataURLToByteArray(contents)).toThrow(Error)

contents = 'dat:;base64,1234567'
expect(() => base64DataURLToByteArray(contents)).toThrow(Error)
})

test('returns Uint8Array if data URL prefix is correct', () => {
// base64 encoding of binary 'm'
// btoa('m') == 'bQ=='
const contents = 'data:mime;base64,bQ=='
expect(base64DataURLToByteArray(contents)).toEqual(new Uint8Array(['m'.charCodeAt(0)]))
})
})

describe('validatePhoto', () => {
test("returns false when the photo isn't a string", () => {
const input = 1234 as any
expect(validatePhoto(input, 'abc123')).toEqual(false)
})

test("returns false when the photo doesn't have a valid image header", () => {
const input = 'Zm9vYmFy'
expect(validatePhoto(input, 'abc123')).toEqual(false)
})

test('returns false when the photo is missing the magic byte header', () => {
const input = 'data:image/png;base64,Zm9vYmFy'
expect(validatePhoto(input, 'abc123')).toEqual(false)
})

test('returns true when the photo is a valid PNG string', () => {
expect(validatePhoto(VALID_PNG_URI, 'abc123')).toEqual(true)
})

test('returns true when the photo is a valid JPEG string', () => {
expect(validatePhoto(VALID_JPEG_URI, 'abc123')).toEqual(true)
})

test('returns true when the photo is a valid GIF string', () => {
expect(validatePhoto(VALID_GIF_URI, 'abc123')).toEqual(true)
})

test('returns false when the photo is larger than 200KB', () => {
expect(validatePhoto(LARGE_IMG_URI, 'abc123')).toEqual(false)
})
})
Loading

0 comments on commit 233725f

Please sign in to comment.