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

Add validation for exported ActivityPub tarballs #7

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

0marSalah
Copy link
Collaborator

No description provided.

console.log(JSON.stringify(result.errors))
})

// it('should fail if outbox.json is missing', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

These tests look useful. Can we include them uncommented?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah i will but it will add more tar files to the src, do you have a better way instead having them in the source code?

* @param tarBuffer - A Buffer containing the .tar archive.
* @returns A promise that resolves to an object with `valid` (boolean) and `errors` (string[]).
*/
export async function validateExportStream(
Copy link
Member

Choose a reason for hiding this comment

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

Will you please make it so tarBuffer can be a ReadableStream? That way, if the export is really big and the tar is really big, it doesn't have to be buffered in memory all at once.

I think you should be able have tar-stream parse the stream, async iterate through the tar entries, and ensure each entry is valid, all without every buffering all the entries in memory

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you r abs right, i should consider that, thanks

@gobengo
Copy link
Member

gobengo commented Jan 8, 2025

I think right now the validateExportStream function isn't exported in a way that packages that depend on this one can import it and use it. If that's true, will you please add it as a package export?

@0marSalah
Copy link
Collaborator Author

0marSalah commented Jan 9, 2025

@gobengo
yeah i am sorry i forgot convert the PR into draft, i was hoping only to look at the function logic, which you do

of course its not done and not in the build directory yet, also i should test it locally too

@0marSalah 0marSalah marked this pull request as draft January 9, 2025 11:59
@0marSalah
Copy link
Collaborator Author

related PR did-coop/hollo#17

i have tested locally with hollo and everything working as expected

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