Skip to content

Mime Parsing#447

Closed
bwhitn wants to merge 33 commits intogchq:masterfrom
bwhitn:eml
Closed

Mime Parsing#447
bwhitn wants to merge 33 commits intogchq:masterfrom
bwhitn:eml

Conversation

@bwhitn
Copy link
Copy Markdown
Contributor

@bwhitn bwhitn commented Dec 19, 2018

This should add Mime Parsing to included Mime Encoded Word Parsing.

This attempts to decode mime reguardless if it is \r\n (correct newline) or \n (incorrect)

Both Base64 and QuotedPrintable is used for decode. Others can be added later but currently these are the supported ones. UUencodeding and BinHex appear to be common.

Copy link
Copy Markdown
Member

@d98762625 d98762625 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @bwhitn - My review is from a 'readbility' point of view: would someone reading through this code in the future find it sensible and easy to understand?

}
dataObj.forEach(function(obj) {
if (obj.body) {
let name = obj.fields.filename ? obj.fields.filename : obj.fields.name;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Here you test for filename but not for the fallback, name. is name guaranteed to exist?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see the name test below now. This could be let name = obj.fields.filename || obj.fields.name.

dataObj.forEach(function(obj) {
if (obj.body) {
let name = obj.fields.filename ? obj.fields.filename : obj.fields.name;
const type = obj.fields.type ? obj.fields.type : "text/plain";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This could become const type = obj.fields.type || "text/plain" for brevity

/**
* Preferred MIME encoding format mappings.
*/
export const MIME_FORMAT = {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would make me so happy to have all these values aligned

/**
* Extract data from mimeObjects and return object array containing them.
* extractData([["testa", "header", "subheader"], ["testb", "header"]]) would
* returns an array of objects {fields: {testa: "somestringornull", testb: "somestringornull"}, header: "somestringornull", body: "somestringornull"}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

minor typo here

*
* @param {Object} mimeObj
*/
static decodeMimeMessage(mimeObj) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In the description here you say 'Attempts to..' - what is the consequence of it failing to decode? Does it just leave the mimeObj's data encoded?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh I see the _decodeMimeData throws. Please can you add a @throws statement to the function comment? Its helpful for reviewers like me now and in the future.

input = Utils.convertToByteArray(input, "base64");
break;
case "quoted-printable":
input = decodeQuotedPrintable(input);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you need a default case here?

* @param {string} boundary
* @return {string[]}
*/
static *_splitMultipart(input, boundary) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's this star? I've never seen that before

* @param {string} input
* @returns {byteArray}
*/
export function decodeQuotedPrintable(input) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This function comment has no summary

*
*
*/
run(input, args) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hilariously long, empty multiline comment 😆

*/
import TestRegister from "../TestRegister";

TestRegister.addTests([
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would be nice to see some more tests here for both Decode Mime Encoded Words and Parse Mime

* @license Apache-2.0
*/
class Mime {
/**
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.

not needed?

input = Utils.strToByteArray(cptable.utils.decode(MIME_FORMAT[charEnc.toLowerCase()], input));
}
return input;
} catch (err) {
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.

you can do catch without err parameter, at least underscore it?

*/
class DecodeMimeEncodedWords extends Operation {

/**
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.

class name says the function of the constructor

@a3957273
Copy link
Copy Markdown
Member

Hey! This pull request has gotten rather old and that's our fault. This project stopped being actively maintained for a while and it looks like your pull request has started to gather dust. Would you be able to update your branch to the latest version of CyberChef and we'll give it a review?

@a3957273 a3957273 closed this Apr 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority tidying required Code improvements needed before merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants