Skip to content

Security patches, fix broken test and tiny nix flake change - #896

Merged
ignatiusm merged 17 commits into
devfrom
895-security-patch-and-broken-test
Jul 29, 2026
Merged

Security patches, fix broken test and tiny nix flake change#896
ignatiusm merged 17 commits into
devfrom
895-security-patch-and-broken-test

Conversation

@ignatiusm

Copy link
Copy Markdown
Contributor

Resolves #895

@ignatiusm

Copy link
Copy Markdown
Contributor Author

Security checks now passing (I kicked of a CI run from this branch):

image

@ignatiusm

Copy link
Copy Markdown
Contributor Author

Note @iamtanuj18 @plouka13, if we upgrade react-router to v8 in the future it will involve removing react-router-dom and some code changes: https://github.com/remix-run/react-router/blob/main/CHANGELOG.md

@ignatiusm

Copy link
Copy Markdown
Contributor Author

Note: I had to upgrade the sharp library due to CVE. This new version likely was bundled with a different version of the underlying libvips binary and codecs, and as a result the image hashes after resizing were different. I didn't want to just blindly update the test data with the hash values printing in the failing tests so I used this following snippet to invoke the same version of sharp that was installed, and use the same resize and hash commands to verify the output:

❯ node -e "
const sharp = require('sharp');
const crypto = require('crypto');
const fs = require('fs');
const input = fs.readFileSync('application/common/testing/fixtures/valid_logo.png');   // the SAME fixture the test uses
const original = crypto.createHash('md5').update(input).digest('hex');
sharp(input).resize(200).png().toBuffer().then(buf => {
  const resized = crypto.createHash('md5').update(buf).digest('hex');
  console.log('originalHash', original);
  console.log('resizedHash ', resized);
});
"
originalHash 096eb6d90cb757f8eb1a3d2fc4008e3e
resizedHash  e0ee98ef4e1db2b2c9c1c3c45508fb81

This illustrates that using hashes is a slightly brittle way to test the logo resize function... but I don't have bandwidth to think about a more fuzzy way to verify this at the moment 🤷 If you have ideas feel free to create an issue and suggest them

@ignatiusm
ignatiusm requested a review from plouka13 July 28, 2026 23:24
@ignatiusm ignatiusm changed the title Security patchs, fix broken test and tiny nix flake change Security patches, fix broken test and tiny nix flake change Jul 28, 2026

@plouka13 plouka13 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No issues with the code. Tested locally everything seems to work as expected. LGTM!

const { id } = useResource()

const { queryResult } = useShow({ resource: 'surveys' })
const { id: surveyId } = useParams()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good name change choice - confused about which resource id it was before the change.

import { useSurveyStore } from '../../surveyStore'
import { axiosInstance } from '../../providers/dataProvider'
import { useResource, useShow, useUpdate, useNavigation, useNotification } from '@refinedev/core'
import { useShow, useUpdate, useNavigation, useNotification } from '@refinedev/core'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seems like a lot of these hooks are re-implementations of the react hooks from this change. Do you reckon there's a need to move away from some of these custom hooks to avoid dependency on refine?

Out of scope for this PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah - I don't have a strong view on this at the moment. Would have to have a think about it 🤔

@ignatiusm
ignatiusm merged commit 7919de6 into dev Jul 29, 2026
12 of 13 checks passed
@ignatiusm
ignatiusm deleted the 895-security-patch-and-broken-test branch July 29, 2026 04:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants