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

[LOW] [Splits] [$500] Mweb/Chrome - Scan request created using corrupt pdf displays incorrect error message #34032

Closed
1 of 6 tasks
lanitochka17 opened this issue Jan 5, 2024 · 85 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Reviewing Has a PR in review Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Jan 5, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 1.4.22
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Issue found when executing PR #32562

Action Performed:

  1. Go to https://staging.new.expensify.com/
  2. Tap fab --- Request money ---Scan
  3. Upload a corrupt pdf
  4. Tap request
  5. Note no error displayed for uploading corrupt pdf
  6. Tap on created scan request
  7. Enter all fields required

Expected Result:

Scan request created using corrupt pdf must display correct error message

Actual Result:

Scan request created using corrupt pdf displays incorrect error message " unexpected error while making request"

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6333288_1704467997273.income.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~019826bd315885912e
  • Upwork Job ID: 1743294656335773696
  • Last Price Increase: 2024-04-16
@lanitochka17 lanitochka17 added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jan 5, 2024
@melvin-bot melvin-bot bot changed the title Mweb/Chrome - Scan request created using corrupt pdf displays incorrect error message [$500] Mweb/Chrome - Scan request created using corrupt pdf displays incorrect error message Jan 5, 2024
Copy link

melvin-bot bot commented Jan 5, 2024

Job added to Upwork: https://www.upwork.com/jobs/~019826bd315885912e

Copy link

melvin-bot bot commented Jan 5, 2024

Triggered auto assignment to @sonialiap (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 5, 2024
Copy link

melvin-bot bot commented Jan 5, 2024

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

Copy link

melvin-bot bot commented Jan 5, 2024

Triggered auto assignment to Contributor-plus team member for initial proposal review - @situchan (External)

@codebycarlos
Copy link

I can't replicate this on my android mobile (I get the error post upload as expected). Could you please share the file?

I believe the receipt is validated here.

function validateReceipt(file) {
const {fileExtension} = FileUtils.splitExtensionFromFileName(lodashGet(file, 'name', ''));
if (!CONST.API_ATTACHMENT_VALIDATIONS.ALLOWED_RECEIPT_EXTENSIONS.includes(fileExtension.toLowerCase())) {
setUploadReceiptError(true, 'attachmentPicker.wrongFileType', 'attachmentPicker.notAllowedExtension');
return false;
}
if (lodashGet(file, 'size', 0) > CONST.API_ATTACHMENT_VALIDATIONS.MAX_SIZE) {
setUploadReceiptError(true, 'attachmentPicker.attachmentTooLarge', 'attachmentPicker.sizeExceeded');
return false;
}
if (lodashGet(file, 'size', 0) < CONST.API_ATTACHMENT_VALIDATIONS.MIN_SIZE) {
setUploadReceiptError(true, 'attachmentPicker.attachmentTooSmall', 'attachmentPicker.sizeNotMet');
return false;
}
return true;
}

It relies entirely on the metadata, without checking content. So differences in how the mobile operating system interprets the metadata could lead to these corrupted PDFs getting through..

@FlorianWueest
Copy link

Proposal

Please re-state the problem that we are trying to solve in this issue:

Corrupt pdfs can be uploaded as a receipt - and the error message is generic.

What is the root cause of that problem?

There's no specific pdf checking logic to determine whether the file is corrupted. Currently, we only check for size parameters and required extensions:

We check it here:

function validateReceipt(file) {
const {fileExtension} = FileUtils.splitExtensionFromFileName(lodashGet(file, 'name', ''));
if (!CONST.API_ATTACHMENT_VALIDATIONS.ALLOWED_RECEIPT_EXTENSIONS.includes(fileExtension.toLowerCase())) {
setUploadReceiptError(true, 'attachmentPicker.wrongFileType', 'attachmentPicker.notAllowedExtension');
return false;
}
if (lodashGet(file, 'size', 0) > CONST.API_ATTACHMENT_VALIDATIONS.MAX_SIZE) {
setUploadReceiptError(true, 'attachmentPicker.attachmentTooLarge', 'attachmentPicker.sizeExceeded');
return false;
}
if (lodashGet(file, 'size', 0) < CONST.API_ATTACHMENT_VALIDATIONS.MIN_SIZE) {
setUploadReceiptError(true, 'attachmentPicker.attachmentTooSmall', 'attachmentPicker.sizeNotMet');
return false;
}
return true;
}

With those parameters:

App/src/CONST.ts

Lines 51 to 60 in fa533ce

API_ATTACHMENT_VALIDATIONS: {
// 24 megabytes in bytes, this is limit set on servers, do not update without wider internal discussion
MAX_SIZE: 25165824,
// An arbitrary size, but the same minimum as in the PHP layer
MIN_SIZE: 240,
// Allowed extensions for receipts
ALLOWED_RECEIPT_EXTENSIONS: ['jpg', 'jpeg', 'gif', 'png', 'pdf', 'htm', 'html', 'text', 'rtf', 'doc', 'tif', 'tiff', 'msword', 'zip', 'xml', 'message'],
},

What changes do you think we should make in order to solve the problem?

Either install front-end checks (render the pdf using 'react-pdf' and 'react-native-pdf'), to check for file corruption. Or adjust the genericEditFailureMessage to be more meaningful in the case of corrupted pdf file.

@samilabud
Copy link
Contributor

Proposal

Please re-state the problem that we are trying to solve in this issue.

Mweb/Chrome - Scan request created using corrupt pdf displays incorrect error message

What is the root cause of that problem?

We are not checking if the PDF file is corrupted/valid file before uploading in the next components:
https://github.com/Expensify/App/blob/main/src/pages/iou/request/step/IOURequestStepScan/index.js
https://github.com/Expensify/App/blob/main/src/pages/iou/request/step/IOURequestStepScan/index.native.js

What changes do you think we should make in order to solve the problem?

We should validate the PDF before uploading the file in the previously mentioned files and show an error message if PDF is corrupted.

Based on our current PDF libraries:

import {Document, Page, pdfjs} from 'react-pdf/dist/esm/entry.webpack';

Create a function to check the integrity of the file:

import {pdfjs} from 'react-pdf/dist/esm/entry.webpack';

const checkPdfIntegrity = async (file, fileExtension) =>
    new Promise((resolve, reject) => {
        if (fileExtension !== CONST.IOU.FILE_TYPES.PDF) {
            return resolve({success: true});
        }
        const fileReader = new FileReader();
        fileReader.onload = async (event) => {
            const arrayBuffer = event.target && event.target.result;

            if (arrayBuffer) {
                try {
                    const loadingTask = pdfjs.getDocument(event.target.result);
                    const pdfDocument = await loadingTask.promise;
                    resolve({success: true, document: pdfDocument});
                } catch (error) {
                    resolve({success: false});
                }
            }
        };

        fileReader.onerror = (error) => {
            // eslint-disable-next-line prefer-promise-reject-errors
            reject({success: false, error});
        };

        fileReader.readAsArrayBuffer(file);
    });

Then inside of validateReceipt functions:

 const isValidPDF = await checkPdfIntegrity(file, fileExtension);

        if (!isValidPDF.success) {
            setUploadReceiptError(true, 'attachmentPicker.attachmentCorruptedError', 'attachmentPicker.pdfCorruptedError');
            return false;
        }

Also, we should add this last strings to languages files like: https://github.com/Expensify/App/blob/main/src/languages/en.ts
eg:

attachmentPicker: {
      .
      .
      .
        attachmentCorruptedError: 'Attachment is corrupted',
        pdfCorruptedError: 'The PDF file is corrupted',
    },

This could be the result:

Valid.PDF.Files.in.Scan.-.test.mp4

@Christinadobrzyn
Copy link
Contributor

Hey @situchan could you check if this GH is a duplicate - thanks!

@melvin-bot melvin-bot bot added the Overdue label Jan 7, 2024
@situchan
Copy link
Contributor

situchan commented Jan 7, 2024

Hey @situchan could you check if this GH is a duplicate - thanks!

yes, it's a dupe

@melvin-bot melvin-bot bot removed the Overdue label Jan 7, 2024
@ishpaul777
Copy link
Contributor

ishpaul777 commented Jan 9, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

Request Money -Scan - Scan report allows protected PDF upload

What is the root cause of that problem?

We're not checking the PDF is weather the pdf is valid or not before uploadin it, and when BE scans the pdf for further processing it throws error.

What changes do you think we should make in order to solve the problem?

We can add a validation on FE to verify if the pdf file is valid or not using the pdfjs fromreact-pdf

const isPdf = pdfjs.isPdfFile(file.name);
if (isPdf) {
    pdfjs
        .getDocument(fileSource)
        .promise.then(onSuccess)
        .catch(onError);
} else {
    onSuccess();
}

☝️ here onSucess is the code in below LOCs, and we need to create onError which should call setUploadReceiptError with valid translation keys.

IOU.setMoneyRequestReceipt_temporaryForRefactor(transactionID, fileSource, file.name);
if (backTo) {
Navigation.goBack(backTo);
return;
}
// When an existing transaction is being edited (eg. not the create transaction flow)
if (transactionID !== CONST.IOU.OPTIMISTIC_TRANSACTION_ID) {
IOU.replaceReceipt(transactionID, file, fileSource);
Navigation.dismissModal();
return;
}
// If a reportID exists in the report object, it's because the user started this flow from using the + button in the composer
// inside a report. In this case, the participants can be automatically assigned from the report and the user can skip the participants step and go straight
// to the confirm step.
if (report.reportID) {
IOU.setMoneyRequestParticipantsFromReport(transactionID, report);
Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_CONFIRMATION.getRoute(iouType, transactionID, reportID));
return;
}
Navigation.navigate(ROUTES.MONEY_REQUEST_STEP_PARTICIPANTS.getRoute(iouType, transactionID, reportID));

For Native:

For native devices, We will try to render the PDF with zero visibilty using PDF component from react-native-pdf if the load complete we fire onSucess, i.e the image upload function else it throws error. (error copy needs to be decided)

// fires when when file is picked by user
const setReceiptAndNavigate = (file) => {
        if (!validateReceipt(file)) {
            return;
        }
        const {fileExtension} = FileUtils.splitExtensionFromFileName(lodashGet(file, 'name', ''));
        if (fileExtension.toLowerCase() === 'pdf') {
            setPdfFile(file);
            return;
        }
        onLoadComplete(file);
};

// fires when when pdf is rendered successfully by pdf 
const onLoadComplete = (file) => {
        // Store the receipt on the transaction object in Onyx
        IOU.setMoneyRequestReceipt(transactionID, file.uri, file.name, action !== CONST.IOU.ACTION.EDIT);

        if (action === CONST.IOU.ACTION.EDIT) {
            updateScanAndNavigate(file, file.uri);
            return;
        }

        navigateToConfirmationStep();
        setPdfFile(null);
};

// this renders with hidden visibilty
{pdfFile && (
                    <Pdf
                        source={{uri: pdfFile.uri, cache: true}}
                        onLoadComplete={onLoadComplete}
                        onError={() => {
                            Alert.alert(translate('receipt.pdfCorrupted'), translate('receipt.pdfCorruptedDescription'));
                            setPdfFile(null);
                        }}
                        style={styles.invisible}
                    />
)}

@melvin-bot melvin-bot bot added the Overdue label Jan 10, 2024
@sonialiap
Copy link
Contributor

@situchan what do you think of the above proposals?

@melvin-bot melvin-bot bot removed the Overdue label Jan 10, 2024
Copy link

melvin-bot bot commented Jan 12, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@melvin-bot melvin-bot bot added the Overdue label Jan 12, 2024
@situchan
Copy link
Contributor

proposals in review

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jan 12, 2024
@sonialiap
Copy link
Contributor

@situchan any update on your review? :)

@melvin-bot melvin-bot bot removed the Overdue label Jan 15, 2024
@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Apr 5, 2024
@kubabutkiewicz
Copy link
Contributor

Proposal

Please re-state the problem that we are trying to solve in this issue.

When in money request -> scan, and uploading a pdf file which is corrupted we are allowing user to move through whole flow till we upload file on the backend. We would like to show error as soon as possible to not waste time of users.

What is the root cause of that problem?

There is no logic implemented in the code for checking if pdfs are corrupted.

What changes do you think we should make in order to solve the problem?

To accomplish this on the web and desktop we can use library called pdfjs which a web-standard for working with pdfs.
We can create a function isValidPdf in

https://github.com/Expensify/App/blob/ce06355873778a69c78e40e0f33e4648f3462826/src/pages/iou/request/step/IOURequestStepScan/index.js#L178-L196which

    const isValidPdfFile = (file) =>
        new Promise((resolve) => {
            pdfjs
                .getDocument(file.uri || '')
                .promise.then(() => resolve(true))
                .catch(() => resolve(false));
        });

On the native side things are trickier because there is no standard with working with pdfs. After research I couldn't find and solution in react-native world so I decided to go with creating native modules. For android code can look like this

 public static boolean isPdfCorrupted(String filePath) {
        try {
            // Read the content of the PDF file into a byte array
            File file = new File(filePath);
            byte[] buffer = new byte[(int) file.length()];
            FileInputStream fis = new FileInputStream(file);
            fis.read(buffer);
            fis.close();

            // Check if the PDF file starts with the "%PDF" header
            String pdfHeader = new String(buffer, 0, 4);
            return !pdfHeader.equals("%PDF");
        } catch (IOException e) {
            // Handle IO errors
            e.printStackTrace();
            return true; // Treat IO errors as corrupted PDF files
        }
    }

    @ReactMethod
    public void checkPdf(String name, Callback callback) {
        if (PdfUtils.isPdfCorrupted(name)) {
            // Handle the case where the PDF file is corrupted
            callback.invoke(false);
        } else {
            // Proceed with processing the PDF file
            // For example, display the PDF file to the user
            callback.invoke(true);
        }
    }

For iOS we don't need any external library we can create function like that

RCT_EXPORT_METHOD(checkPdf:(NSString *)fileurl callback: (RCTResponseSenderBlock)callback)
{
  NSURL *url = [NSURL URLWithString:fileurl];
  NSFileManager *fileManager = [NSFileManager defaultManager];
      NSData *ns = [NSData dataWithContentsOfURL:url];
      BOOL fileExists = [fileManager fileExistsAtPath:[url path]];
      BOOL isPDF = NO;
      if ([ns length] >= 1024) 
      {
          uint8_t pdfBytes[] = { 0x25, 0x50, 0x44, 0x46 };
          NSData *pdfHeader = [NSData dataWithBytes:pdfBytes length:4];
          NSRange range = [ns rangeOfData:pdfHeader options:NSDataSearchAnchored range:NSMakeRange(0, 1024)];
          if (range.length > 0)
          {
            callback(@[@YES]);
          }
          else
          {
            callback(@[@NO]);
          }
      }
}

And in our codebase use it here

const validateReceipt = (file) => {
const {fileExtension} = FileUtils.splitExtensionFromFileName(lodashGet(file, 'name', ''));
if (!CONST.API_ATTACHMENT_VALIDATIONS.ALLOWED_RECEIPT_EXTENSIONS.includes(fileExtension.toLowerCase())) {
Alert.alert(translate('attachmentPicker.wrongFileType'), translate('attachmentPicker.notAllowedExtension'));
return false;
}
if (lodashGet(file, 'size', 0) > CONST.API_ATTACHMENT_VALIDATIONS.MAX_SIZE) {
Alert.alert(translate('attachmentPicker.attachmentTooLarge'), translate('attachmentPicker.sizeExceeded'));
return false;
}
if (lodashGet(file, 'size', 0) < CONST.API_ATTACHMENT_VALIDATIONS.MIN_SIZE) {
Alert.alert(translate('attachmentPicker.attachmentTooSmall'), translate('attachmentPicker.sizeNotMet'));
return false;
}
return true;
};

the code would look like that

 const validateReceipt = (file) =>
        new Promise((resolve) => {
            CheckPDFDocument.checkPdf(file.uri, (isCorrect) => {
                const {fileExtension} = FileUtils.splitExtensionFromFileName(lodashGet(file, 'name', ''));
                if (!isCorrect) {
                    Alert.alert(translate('attachmentPicker.wrongFileType'), translate('attachmentPicker.notAllowedExtension'));
                    resolve(false);
                }

                if (!CONST.API_ATTACHMENT_VALIDATIONS.ALLOWED_RECEIPT_EXTENSIONS.includes(fileExtension.toLowerCase())) {
                    Alert.alert(translate('attachmentPicker.wrongFileType'), translate('attachmentPicker.notAllowedExtension'));
                    resolve(false);
                }

                if (lodashGet(file, 'size', 0) > CONST.API_ATTACHMENT_VALIDATIONS.MAX_SIZE) {
                    Alert.alert(translate('attachmentPicker.attachmentTooLarge'), translate('attachmentPicker.sizeExceeded'));
                    resolve(false);
                }

                if (lodashGet(file, 'size', 0) < CONST.API_ATTACHMENT_VALIDATIONS.MIN_SIZE) {
                    Alert.alert(translate('attachmentPicker.attachmentTooSmall'), translate('attachmentPicker.sizeNotMet'));
                    resolve(false);
                }
                resolve(true);
            });
        });

What alternative solutions did you explore? (Optional)

We can change a little bit a UI for the first step in the flow of adding scan while requesting a money and add preview of the document/image and use PDFView component and then if we cant display the preview of the pdf/image we would know that there is something wrong with the file

@melvin-bot melvin-bot bot removed the Overdue label Apr 9, 2024
Copy link

melvin-bot bot commented Apr 9, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@rushatgabhane
Copy link
Member

@kubabutkiewicz 's proposal LGTM!

The native code is really easy to read and understand, so I don't mind us creating a native module to check for valid PDFs

@rushatgabhane
Copy link
Member

🎀👀🎀

Copy link

melvin-bot bot commented Apr 9, 2024

Triggered auto assignment to @luacmartins, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@luacmartins
Copy link
Contributor

@kubabutkiewicz I see that your proposal suggests using a lib for web and building our own code for ios/android. I'm pretty sure that if we go ahead with the new lib request, people are gonna veto it in favor of writing our own for web too. Could you explore writing our own method for web as well?

@kubabutkiewicz
Copy link
Contributor

@luacmartins actually this lib is already added to our project , so no need to add new one 😄 pdfjs is used in react-pdf and we can use it like that import {pdfjs} from 'react-pdf';

@luacmartins
Copy link
Contributor

luacmartins commented Apr 10, 2024

Ah interesting. In that case the proposal looks good then. Let's go ahead with the PR.

@melvin-bot melvin-bot bot added the Overdue label Apr 15, 2024
@luacmartins
Copy link
Contributor

@kubabutkiewicz do you have an ETA for the PR?

@melvin-bot melvin-bot bot removed the Overdue label Apr 15, 2024
@kubabutkiewicz
Copy link
Contributor

Yes, I think tomorrow will be ready, today was reviewed internally, I will make adjustments and when approved internally I will open to external review

@kubabutkiewicz
Copy link
Contributor

kubabutkiewicz commented Apr 16, 2024

Hi I noticed on latest main that there is a bug that even when I am uploading pdf which is not corrupted I am getting error that file is corrupted and I think its related to this #40162 PR because of that I cant test newest changes

Copy link

melvin-bot bot commented Apr 16, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@melvin-bot melvin-bot bot added the Overdue label Apr 18, 2024
@rushatgabhane
Copy link
Member

rushatgabhane commented Apr 18, 2024

because of that I cant test newest changes

thank you for finding the cause. let's hold on #40471 (review)

@melvin-bot melvin-bot bot removed the Overdue label Apr 18, 2024
@paultsimura
Copy link
Contributor

paultsimura commented Apr 19, 2024

@rushatgabhane you shouldn't hold for #40471 – the mentioned issue was fixed in #40351, which is already merged.

@kubabutkiewicz please retry with the latest main.

@kubabutkiewicz
Copy link
Contributor

Hi, just wanted to inform that I will be OOO from 29.04 to 05.05.

@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

@luacmartins
Copy link
Contributor

Seems like #40471 (review) may have fixed the issue. Gonna close it since it's no longer reproducible. Please reopen if this is still happening.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests