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

feat(safer): validate files with Safer #4421

Merged
merged 5 commits into from
Sep 1, 2022
Merged

feat(safer): validate files with Safer #4421

merged 5 commits into from
Sep 1, 2022

Conversation

maljuburi
Copy link
Contributor

Which issue(s) this PR fixes 🔨
AP-2186

@github-actions github-actions bot added the Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA label Aug 23, 2022
<ui-image-container
v-else
class="image-container"
:nsfw="image.nsfw && blockNsfw"
Copy link
Contributor

@josephmcg josephmcg Aug 24, 2022

Choose a reason for hiding this comment

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

we need to check the blockNsfw user setting to determine whether they want to see nsfw content. This current change would always blur if its nsfw, regardless of user setting. Although, we could probably add the settings check to the image-container component, rather than doing it at the parent level every time

-- edit: I now see this specific component was referencing the old textile state. example of new implementation can be found here
components/views/chat/chatbar/upload/preview/item/Item.vue

Copy link
Contributor Author

@maljuburi maljuburi Aug 29, 2022

Choose a reason for hiding this comment

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

Thanks @josephmcg for the review, I haven't looked at the front-end side of Safer yet, this was just me working around enabling a user to send a file in chat so I can test the iridium side, since that wasn't fully working at the time of this branch's creation. but I will look into the new implementation you suggested soon. 👍🏻 Thanks again!

@josephmcg josephmcg added Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. and removed Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA labels Aug 24, 2022
@stavares843 stavares843 linked an issue Aug 29, 2022 that may be closed by this pull request
Copy link
Contributor

@aewing aewing left a comment

Choose a reason for hiding this comment

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

If this doesn't break anything I would be in favor of merging it and following up with another UI ticket

@aewing aewing removed the Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. label Aug 31, 2022
@josephmcg
Copy link
Contributor

josephmcg commented Aug 31, 2022

why switch thumbnail from cid to the blob itself? the utility function only returns a scaled down version in the case of image files. If no thumbnail can be generated, it returns undefined. This has potential to store undefined inside iridium (which I believe breaks things)

@josephmcg josephmcg added the Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. label Aug 31, 2022
@maljuburi
Copy link
Contributor Author

maljuburi commented Aug 31, 2022

For the thumbnail, my thought was to not store 2 files ( original image + thumbnail ) in the sync-node . Instead I thought thumbnails should only be a manager/front-end utility that resizes the original image we store in sync-node imo. right?

@maljuburi
Copy link
Contributor Author

Maybe thumbnail shouldn't be in the attachment all together... I don't know!
I think we should only send the original image to the users (after it gets Safer checked).
And then, the users generate its thumbnail.

@tooshel tooshel removed the Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. label Sep 1, 2022
@maljuburi maljuburi merged commit 2f7bc69 into dev Sep 1, 2022
@maljuburi maljuburi deleted the AP-2186 branch September 1, 2022 00:56
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.

AP-2186: Sync Node / File Sharing / Safer
4 participants