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
4 changes: 2 additions & 2 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ jobs:
- run: npm run test-ci
env:
NODE_OPTIONS: '--max-old-space-size=8192'
- name: Submit test coverage to Coveralls
uses: coverallsapp/github-action@v1.1.2
- name: Submit coverage to Coveralls
uses: coverallsapp/github-action@v2
with:
github-token: ${{ secrets.GITHUB_TOKEN }}
20,269 changes: 8,119 additions & 12,150 deletions package-lock.json

Large diffs are not rendered by default.

3 changes: 1 addition & 2 deletions package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@opengovsg/formsg-sdk",
"version": "0.13.0",
"version": "0.14.0",
"repository": {
"type": "git",
"url": "https://github.com/opengovsg/formsg-javascript-sdk.git"
Expand Down Expand Up @@ -36,7 +36,6 @@
"@types/node": "^18.18.9",
"@typescript-eslint/eslint-plugin": "^4.25.0",
"auto-changelog": "^2.4.0",
"coveralls": "^3.1.1",

Choose a reason for hiding this comment

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

praise: Thanks for identifying and removing this! 🎉 Always happy to remove stuff -> less things to maintain!

"eslint-config-prettier": "^8.3.0",
"eslint-plugin-import": "^2.23.3",
"eslint-plugin-jest": "^24.3.6",
Expand Down
29 changes: 27 additions & 2 deletions spec/crypto-v3.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,27 @@ import {
ciphertext,
formPublicKey,
formSecretKey,
submissionSecretKey
submissionSecretKey,
plainVerifiedText
} from './resources/crypto-v3-data-20231207'
import CryptoV3 from '../src/crypto-v3'
import Crypto from '../src/crypto'
import { SIGNING_KEYS } from '../src/resource/signing-keys'

const INTERNAL_TEST_VERSION = 3

const testFileBuffer = new Uint8Array(Buffer.from('./resources/ogp.svg'))

const encryptionPublicKey = SIGNING_KEYS.test.publicKey
const signingSecretKey = SIGNING_KEYS.test.secretKey

jest.mock('axios', () => mockAxios)

describe('CryptoV3', function () {
afterEach(() => mockAxios.reset())

const crypto = new CryptoV3()
const crypto = new CryptoV3({ signingPublicKey: encryptionPublicKey })
const cryptoV1 = new Crypto({ signingPublicKey: encryptionPublicKey })

it('should generate a keypair', () => {
const keypair = crypto.generate()
Expand Down Expand Up @@ -126,4 +133,22 @@ describe('CryptoV3', function () {

expect(decrypted).toBeNull()
})

it('should be able to encrypt and decrypt submissions with verifiedContent from 2023-12-07 end-to-end successfully from the form private key', () => {
// Arrange
const { publicKey, secretKey } = crypto.generate()

// Act
const ciphertext = crypto.encrypt(plaintext, publicKey)
const verifiedText = cryptoV1.encrypt(plainVerifiedText, ciphertext.submissionPublicKey, signingSecretKey)
Copy link

@kevin9foong kevin9foong Sep 29, 2025

Choose a reason for hiding this comment

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

Suggestion: Could we perhaps leave a comment about the rationale behind why cryptoV1 is used for decrypting verifiedContent here?
It might be a little confusing as to why cryptoV1 is being used in the cryptoV3 test file for new engs.

Perhaps something explaining the below comment (but probably more concise).

Choose a reason for hiding this comment

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

Question - confirm if this is accurate:
For storage mode forms, the verifiedContent is added outside of the main encryptedContent.

For MRF, this verifiedContent is not yet implemented.
Thinking about its implementation and extensibility to MRF singpass for multiple steps. (crucial since changes to SDK usually can be added but hard to be taken away).

For singpass with MRF on the FormSG app side, my understanding is that since it is only for 1st step - we can use cryptoV1 to encrypt with the current step's submission public key.
For multiple step MRFs, we can decrypt with the prev step's submission public key and then re-encrypt for each subsequent step with the latest submission public key.

However, if we use cryptoV1 for verified content and use the submission steps's latest submission key - would admins still be able to decrypt the verified content with the form private key using decrypt in cryptoV3?

  • yes, since can pass in {encryptedSubmissionSecretKey, ...verifiedContent} as the DecryptParams and the same encryptMessage is used in v1 and v3 - allowing the decrypt in v3 to also decrypt ciphertexts using v1.

Hence, this change is safe to make. But there likely will need to be explicit logic to do this:

For multiple step MRFs, we can decrypt with the prev step's submission public key and then re-encrypt for each subsequent step with the latest submission public key. 

in the application code.

const decrypted = crypto.decrypt(secretKey, {
...ciphertext,
verifiedContent: verifiedText,
version: INTERNAL_TEST_VERSION,
})
// Assert
expect(decrypted).toHaveProperty('responses', plaintext)
expect(decrypted).toHaveProperty('verified', plainVerifiedText)

})
})
3 changes: 3 additions & 0 deletions spec/resources/crypto-v3-data-20231207.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ const plaintext = {
}
}

const plainVerifiedText = { 'uinFin (Step 1)': 'S9912370B' }

const ciphertext = {
encryptedContent:
'yUW5li4+IA9q2/n3ZS+5+wrXQ8mKGrFJ1KW9Kf/eRzc=;PgZE8+y8rBvssnqLnqjnnqHDW6PngYKK:eIEuOUQjf1YkQIulZ7bCKXIl6wByg644Ulk/LjhefmLzhkVmXbTxBJVKVG6YgV0ZMcG4JPUuQ+WOW+N1/AOyL/8DJqclX74kG6s0DNXIJixkqNZCnfZapulerR9XXKSfwBjpo1nK25KCg32F/ey2HypPcluGV19hWwgj80mlms7Ya7x1X5wcdttlGrzGEnNH2VEPXjzJZHqiV1TWoQGwxSZ753fpkHUkBeKFA1UkMHS5XYnWyYD48JpfpOAz0L2ti6RHQnQLSKUHscYVfAZt5OyUGqPFmhm2ulWdycNVp8HayQrpqeY8cdu8QsmZRdNCMfMFLahZCm6xKS+8GUrJWgJr64yaZpkxQS45uPb9zxC+G/u4FZhS/YsrjDTuIIwMGS0+qsNr4075yemFFAQHIpbhWZ9QlYrNq2TAolrVezeAw3AQ/nr4sz60dvqRahcse9x8oMxB7jA55OuxH5uk6PcCIAmEi+njr6Lgbcn2mtPMyk7kGcwjNzCL57b51RxJVi0ZqNXrS0FFepvzCK3IOEqKqrKGGK0qGqF4MFsH2wdq4RFkXjLMZk4u9ZWjIRjc',
Expand All @@ -44,6 +46,7 @@ const submissionSecretKey = 'bIyKphcx5hiuBaJ4q5cwnXaFNY9Ofe5NQBqTEzf3zYA='

export {
plaintext,
plainVerifiedText,
ciphertext,
formPublicKey,
formSecretKey,
Expand Down
84 changes: 71 additions & 13 deletions src/crypto-v3.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,15 @@ import {
encodeUTF8,
} from 'tweetnacl-util'

import { decryptContent, encryptMessage, generateKeypair } from './util/crypto'
import {
decryptContent,
encryptMessage,
generateKeypair,
verifySignedMessage,
} from './util/crypto'
import { determineIsFormFieldsV3 } from './util/validate'
import CryptoBase from './crypto-base'
import { MissingPublicKeyError } from './errors'
import {
DecryptedContentV3,
DecryptParams,
Expand All @@ -17,8 +23,11 @@ import {
} from './types'

export default class CryptoV3 extends CryptoBase {
constructor() {
signingPublicKey?: string

constructor({ signingPublicKey }: { signingPublicKey?: string } = {}) {
super()
this.signingPublicKey = signingPublicKey
}

/**
Expand Down Expand Up @@ -62,7 +71,7 @@ export default class CryptoV3 extends CryptoBase {
decryptParams: DecryptParams
): DecryptedContentV3 | null => {
try {
const { encryptedContent } = decryptParams
const { encryptedContent, verifiedContent } = decryptParams

// Do not return the transformed object in `_decrypt` function as a signed
// object is not encoded in UTF8 and is encoded in Base-64 instead.
Expand All @@ -85,8 +94,47 @@ export default class CryptoV3 extends CryptoBase {
responses: decryptedObject as FormFieldsV3,
}

/**
* Note on verifiedContent decryption for cryptoV3:

Choose a reason for hiding this comment

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

Suggested change
* Note on verifiedContent decryption for cryptoV3:
* Note on verifiedContent decryption for cryptoV3:
* Although decryption is supported, verifiedContent encryption is not supported
* in cryptoV3 encrypt.
* This is to keep the encryption of verifiedContent and encryptedContent similar to storage mode - where
* verifiedContent and encryptedContent are defined and encrypted separately.

* Although decryption is supported, verifiedContent encryption is not supported
* in cryptoV3 encrypt.
* This is to keep the encryption of verifiedContent and encryptedContent similar to storage mode - where
* verifiedContent and encryptedContent are defined and encrypted separately.
*/
// decrypt verifiedContent if it exists
if (verifiedContent) {
if (!this.signingPublicKey) {
throw new MissingPublicKeyError(
'Public signing key must be provided when instantiating the Crypto class in order to verify verified content'
)
}

const decryptedVerifiedContent = decryptContent(
submissionSecretKey,
verifiedContent
)

if (!decryptedVerifiedContent) {
// Returns null if decrypting verified content failed.
throw new Error('Failed to decrypt verified content')
}

const decryptedVerifiedObject = verifySignedMessage(
decryptedVerifiedContent,
this.signingPublicKey
)

returnedObject.verified = decryptedVerifiedObject
}

return returnedObject
} catch (err) {
// Should only throw if MissingPublicKeyError.
// This library should be able to be used to encrypt and decrypt content
// if the content does not contain verified fields.
if (err instanceof MissingPublicKeyError) {
throw err
}
return null

Choose a reason for hiding this comment

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

question: in crypto.ts,

    } catch (err) {
      // Should only throw if MissingPublicKeyError.
      // This library should be able to be used to encrypt and decrypt content
      // if the content does not contain verified fields.
      if (err instanceof MissingPublicKeyError) {
        throw err
      }
      return null
    }

The missing public key error is thrown to the client - should we perhaps be doing the same thing for cryptov3?

Copy link
Contributor Author

@scottheng96 scottheng96 Sep 29, 2025

Choose a reason for hiding this comment

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

Good idea to keep it consistent between the versions. I'll throw the error from decryptFromSubmissionKey and to decrypt as well.

Tbh We don't handle these errors in our code, where we only check for null and return another error else. But that's for later when we update app implementation

}
}
Expand All @@ -99,24 +147,34 @@ export default class CryptoV3 extends CryptoBase {
* @param decryptParams.encryptedSubmissionSecretKey The encrypted submission secret key encoded with base-64.
* @param decryptParams.version The version of the payload. Used to determine the decryption process to decrypt the content with.
* @returns The decrypted content if successful. Else, null will be returned.
* @throws {MissingPublicKeyError} if a public key is not provided when instantiating this class and is needed for verifying signed content.
*/
decrypt = (
formSecretKey: string,
decryptParams: DecryptParamsV3
): DecryptedContentV3 | null => {
const { encryptedSubmissionSecretKey, ...rest } = decryptParams
try {
const { encryptedSubmissionSecretKey, ...rest } = decryptParams

const submissionSecretKey = decryptContent(
formSecretKey,
encryptedSubmissionSecretKey
)
const submissionSecretKey = decryptContent(
formSecretKey,
encryptedSubmissionSecretKey
)

if (submissionSecretKey === null) return null
if (submissionSecretKey === null) return null

return this.decryptFromSubmissionKey(
encodeBase64(submissionSecretKey),
rest
)
return this.decryptFromSubmissionKey(
encodeBase64(submissionSecretKey),
rest
)

} catch (err) {
if (err instanceof MissingPublicKeyError) {
// rethrow to let the caller decide how to handle missing signing key
throw err
}
return null
}
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export = function (config: PackageInitParams = {}) {
secretKey: webhookSecretKey,
}),
crypto: new Crypto({ signingPublicKey }),
cryptoV3: new CryptoV3(),
cryptoV3: new CryptoV3({ signingPublicKey }),
verification: new Verification({
publicKey: verificationPublicKey,
secretKey: verificationOptions?.secretKey,
Expand Down
2 changes: 2 additions & 0 deletions src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ export interface DecryptParams {
export interface DecryptParamsV3 {
encryptedContent: EncryptedContent
encryptedSubmissionSecretKey: EncryptedContent
verifiedContent?: EncryptedContent
version: number
}

Expand All @@ -93,6 +94,7 @@ export type DecryptedContent = {
export type DecryptedContentV3 = {
submissionSecretKey: string
responses: FormFieldsV3
verified?: Record<string, any>
}

export type DecryptedFile = {
Expand Down