Skip to content
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.

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

fix: Allow JPEG and GIF images as profile photos #2332 #2353

Merged
merged 22 commits into from
Mar 14, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
34ca007
Allow JPEG and GIF images as profile photos
Mar 11, 2024
c7c7877
Update CHANGELOG files
Mar 11, 2024
22dc503
Accidentally included bad string
Mar 11, 2024
53aaeed
Add root-level lint script and fix linting errors
Mar 11, 2024
5cef04d
Update E2E tests to include JPEG and GIF profile photo tests
Mar 11, 2024
5f4a13b
Update formatting of CHANGELOG files
Mar 11, 2024
18306fe
Refactor profile photo validation
Mar 11, 2024
e62832a
Add comments
Mar 11, 2024
d292fb5
Fix linting (thought I ran this already)
Mar 11, 2024
6f739ff
Update e2e tests and add ability to run tests locally against locally…
islathehut Mar 13, 2024
d55bafc
Add backend tests for gif and jpeg validation
islathehut Mar 13, 2024
f085fa3
Separate test script for local binaries
islathehut Mar 13, 2024
d2c423e
Update naming convention and mild refactor
islathehut Mar 13, 2024
d1d4c33
Add documentation on running tests against local binaries
islathehut Mar 13, 2024
3b94e16
Add unit tests to backend for validatePhoto and mild refactor
islathehut Mar 13, 2024
52dbb8e
Fix tor test issue I was seeing locally, add tor logging and skip run…
islathehut Mar 13, 2024
1b8446e
Linting
islathehut Mar 13, 2024
f2950bf
Merge branch 'develop' into bug/2332-allow-jpeg-profile-photos
islathehut Mar 13, 2024
6d15c87
Fix tests hanging on getting back button
islathehut Mar 14, 2024
26e433c
Linting
islathehut Mar 14, 2024
a698a4d
Use unique data-testids for back arrow (thank you Emilia)
islathehut Mar 14, 2024
a9aca46
Linting
islathehut Mar 14, 2024
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
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,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
4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@
"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"
},
"engines": {
"node": "18.12.1",
Expand Down
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
109 changes: 77 additions & 32 deletions packages/backend/src/nest/storage/userProfile/userProfile.store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,22 +20,93 @@ import { KeyValueIndex } from '../orbitDb/keyValueIndex'

const logger = createLogger('UserProfileStore')

export const checkImgHeader = (buffer: Uint8Array, header: number[]): boolean => {
if (buffer.length < header.length) {
return false
}

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

/**
* 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 checkImgHeader(buffer, pngHeader)
}

/**
* Check magic byte sequence to determine if buffer is a JPEG image.
*/
export const isJpeg = (buffer: Uint8Array): boolean => {
// https://en.wikipedia.org/wiki/JPEG
const jpegHeader = [0xff, 0xd8, 0xff]

return checkImgHeader(buffer, jpegHeader)
}

/**
* Check magic byte sequence to determine if buffer is a GIF image.
*/
export const isGif = (buffer: Uint8Array): boolean => {
// https://en.wikipedia.org/wiki/GIF
// GIF images are different from JPEG and PNG in that there are two slightly different magic number sequences that translate to GIF89a and GIF87a
const gifHeader89 = [0x47, 0x49, 0x46, 0x38, 0x39, 0x61]
const gifHeader87 = [0x47, 0x49, 0x46, 0x38, 0x37, 0x61]
const headers = [gifHeader89, gifHeader87]

for (const header of headers) {
if (checkImgHeader(buffer, header)) {
return true
}
}

return false
}

/**
* Validate a profile photo in a user profile
*
* @param photoString Base64 string representing the photo file that was uploaded
* @param pubKey Public key string for logging purposes
* @returns True if photo is valid and false if not
*/
export const validatePhoto = (photoString: string, pubKey: string): boolean => {
// validate that we have the photo as a base64 string
if (typeof photoString !== 'string') {
logger.error('Expected PNG, JPEG or GIF as base64 string for user profile photo', pubKey)
return false
}

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

// validate that the type is approved and has a matching magic number header
if (
!(photoString.startsWith('data:image/png;base64,') && isPng(photoBytes)) &&
!(photoString.startsWith('data:image/jpeg;base64,') && isJpeg(photoBytes)) &&
!(photoString.startsWith('data:image/gif;base64,') && isGif(photoBytes))
) {
logger.error('Expected valid PNG, JPEG or GIF for user profile photo', 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')
return false
}

return true
}

Expand Down Expand Up @@ -165,33 +236,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
2 changes: 2 additions & 0 deletions packages/desktop/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
[unreleased]

# Fixes

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

[2.1.2]

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ export const EditPhotoButton: FC<{ onChange: (photo?: File) => void }> = ({ onCh
onClick={evt => {
;(evt.target as HTMLInputElement).value = ''
}}
accept='image/png'
accept='image/png, image/jpeg, image/gif'
hidden
/>
</button>
Expand Down
2 changes: 2 additions & 0 deletions packages/e2e-tests/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@

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

* Add tests for JPEG and GIF profile photos

[2.0.3-alpha.6]

* Fix: filter out invalid peer addresses in peer list. Update peer list in localdb.
Expand Down
Binary file added packages/e2e-tests/assets/profile-photo.gif
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added packages/e2e-tests/assets/profile-photo.jpg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
16 changes: 14 additions & 2 deletions packages/e2e-tests/src/selectors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -158,13 +158,25 @@ export class UserProfileContextMenu {
await button.click()
}

async uploadPhoto() {
async uploadPhoto(fileName: string) {
const input = await this.driver.wait(
until.elementLocated(By.xpath('//input[@data-testid="user-profile-edit-photo-input"]'))
)
const filePath = path.join(__dirname, '../assets/profile-photo.png')
const filePath = path.join(__dirname, fileName)
await input.sendKeys(filePath)
}

async uploadPNGPhoto() {
await this.uploadPhoto('../assets/profile-photo.png')
}

async uploadJPEGPhoto() {
await this.uploadPhoto('../assets/profile-photo.jpeg')
}

async uploadGIFPhoto() {
await this.uploadPhoto('../assets/profile-photo.gif')
}
}

export class RegisterUsernameModal {
Expand Down
18 changes: 16 additions & 2 deletions packages/e2e-tests/src/tests/userProfile.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,11 +106,25 @@ describe('User Profile Feature', () => {
await generalChannelOwner.sendMessage(users.owner.messages[0])
})

it('Owner updates their profile photo', async () => {
it('Owner updates their profile photo with PNG', async () => {
const menu = new UserProfileContextMenu(users.owner.app.driver)
await menu.openMenu()
await menu.openEditProfileMenu()
await menu.uploadPhoto()
await menu.uploadPNGPhoto()
})

it('Owner updates their profile photo with JPEG', async () => {
const menu = new UserProfileContextMenu(users.owner.app.driver)
await menu.openMenu()
await menu.openEditProfileMenu()
await menu.uploadJPEGPhoto()
})

it('Owner updates their profile photo with GIF', async () => {
Copy link
Contributor

@EmiM EmiM Mar 12, 2024

Choose a reason for hiding this comment

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

Could you check locally if userProfile test is passing?

  • Last of "Owner updates their profile photo with..." tests uploads gif photo so if the upload suppose to be successful then assertion in "Owner's message with profile photo is visible on channel" should be updated from checking png to gif
  • Could you maybe add assertion at the end of each of "Owner updates their profile photo with..." cases to make sure that e.g users do not see upload error message?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added assertions and testing locally now

const menu = new UserProfileContextMenu(users.owner.app.driver)
await menu.openMenu()
await menu.openEditProfileMenu()
await menu.uploadGIFPhoto()
})

it("Owner's message with profile photo is visible on channel", async () => {
Expand Down
Loading