Grace was tarcking down some production failures, which put her on the path to inspecting a lot of URLs in requests. And that put her onto this blob of code:

app.get(
    (
      [
        "/api/ddm/getProjectList",
        ":dueDate",
        ":status",
        ":userAssignedId",
        ":managerID",
        ":clientID",
        ":projectHeaderID",
        ":tagId",
        ":companyId",
        ":clientGroup",
        ":isDefault",
        ":dateRange",
        ":dateToFilter",
        ":tagIds",
        ":statusIds",
        ":repeatValues",
        ":engagementID?",
        ":completionDate?"
      ]
      .join( "/" )
    ),
    ddmDboardCtrl.getProjectList
);

This defines a route in ExpressJS for handling GET requests. And it defines the route such that every single parameter on the request is contained in the path portion of the URL. That raises questions about why you need seventeen parameters to fulfill your request and what that means for our API design, but it's even worse than it looks: most of those parameters are allowed to be null.

That means the request looks like this:

GET /api/ddm/getProjectList/null/null/null/null/878778/null/null/2049/null/null/null/null/null/null/null/3532061?

For bonus point, the developer responsible for that awful API also has a "special" way for dealing with possibly empty returns:

(
      fs.readdirSync( `${ GLOBAL_DIRECTORY_PATH }` )
  ||
      (
        [ ]
      )
)
.map(
  (
    function( moduleName ){
      return  (
                path.resolve( 
                  ( `${ GLOBAL_DIRECTORY_PATH }/${ moduleName }` )
                )
              );
    }
  )
)

This code calls reddirSync and in case that returns null, ||s the result with an empty array. Only one problem: readdirSync never returns null. It returns an empty array when there are no results.

Also, this indenting is as submitted, which… what is even happening?

This developer has a strange relationship with nulls- defending against them when they're not a risk, spamming them into URLs. They have gazed too long into the null, and the null gazes back into them.

[Advertisement] Keep all your packages and Docker containers in one place, scan for vulnerabilities, and control who can access different feeds. ProGet installs in minutes and has a powerful free version with a lot of great features that you can upgrade when ready.Learn more.

Grace sends us, in her words, "the function that validates the data from the signup form for a cursed application."

It's more than one function, but there are certainly some clearly cursed aspects of the whole thing.

function trimStr(v) {
  return typeof v === "string" ? v.trim() : v;
}

This function, itself, isn't cursed, but it certainly represents a bad omen. Take any type of input, and if that input happens to be a string, return the trimmed version. Otherwise, return the input unchanged. I've got good news and bad news about this omen: the good news is that it isn't used in most of the code that follows, and the bad news is that it is used in some of the code that follows.

The next function builds a validation schema using the yup library, and we'll take this one in chunks, since it's long.

function buildSchema() {
  // Common string with trim transform
  const t = () =>
    yup
      .string()
      .transform((val) => (typeof val === "string" ? val.trim() : val))
      .nullable();

See, I promised that the trimStr function wasn't used in most of the code- because they just copy/pasted its body where they needed it.

  let emailField = yup
    .string()
    .transform((val) => (typeof val === "string" ? val.trim().toLowerCase() : val))
    .nullable()
    .required("email is required");

  emailField = emailField.test("email-format", "email is invalid", (v) => {
      if (!v) return false; // required above
      // Simple email format validation
      return /^[^\s@]+@[^\s@]+\.[^\s@]{2,}$/i.test(v);
    });

I assume t above is meant to be a common base transformation, so you don't have to constantly rewrite the trim functionality. Though this isn't precisely a trim- it also canonicalizes the address to lower case. That will likely work most of the time, but while the domain portion of an email address is case insensitive, the address part of it is not- Remy@somesite.com and remy@somesite.com could be different addresses.

They also make the email field both nullable and required, which is an interesting choice. Not one they're confident about, as they also check that the required field is actually populated in their test function. Then they do a regex to validate the email address, which it's worth noting that email addresses shouldn't be validated by regexes, but also yup already includes an email validation, so none of this is necessary.

let passwordField = yup.string().nullable().required("password is required");

  passwordField = passwordField
    .test(
      "password-min-length",
      "password must be at least 8 characters",
      (v) => !!v && v.length >= 8
    )
    .test(
      "password-alpha-num",
      "password must contain letters and numbers",
      (v) => !!v && (/[A-Za-z]/.test(v) && /\d/.test(v))
    );

  let confirmPasswordField = yup.string().nullable().required("confirmPassword is required");

  confirmPasswordField = confirmPasswordField.test(
      "passwords-match",
      "password and confirmPassword do not match",
      function (v) {
        const pwd = this.parent.password;
        if (!v && !pwd) return false; // both empty => invalid
        if (!v || !pwd) return true; // required rules will handle
        return v === pwd;
      }
    );

Passwords limited to alphanumeric is a choice. A bad one, certainly. Again we also see the pattern of nullable required fields.

  let telephoneField = t().required("telephone is required");

  telephoneField = telephoneField.test("telephone-digits", "telephone is invalid", (v) => {
      if (!v) return false;
      const digits = (v.match(/\d/g) || []).length;
      return digits >= 7;
    });

Oh, at least on phone numbers they use that common base transformation. Again, they're not using the built-in features of yum which can already validate phone numbers, but hey, at least they're making sure that there are at least seven digits, which probably works in some places. Not everywhere, but some places.

  const schema = yup.object().shape({
    firstName: t().required("firstName is required").max(100, "firstName too long"),
    lastName: t().required("lastName is required").max(100, "lastName too long"),
    companyName: t().required("companyName is required").max(150, "companyName too long"),
    telephone: telephoneField,
    email: emailField,
    product: t().max(150, "product too long"),
    password: passwordField,
    confirmPassword: confirmPasswordField,
    affiliateId: t(),
    visitorId: t(),
  });

  return schema;
}

And here we finish constructing the schema, and look at that- we do use that base transformation a few more times here.

How do we use it?

function validateSignupPayload(payload = {}) {

  // Normalize input keys to match schema: support email/emailAddress and telephone/phoneNumber
  const normalized = {
    firstName: trimStr(payload.firstName),
    lastName: trimStr(payload.lastName),
    companyName: trimStr(payload.companyName),
    telephone: trimStr(payload.telephone) || trimStr(payload.phoneNumber),
    email: (trimStr(payload.email) || trimStr(payload.emailAddress) || "").toLowerCase(),
    product: trimStr(payload.product),
    password: typeof payload.password === "string" ? payload.password : payload.password || undefined,
    confirmPassword:
      typeof payload.confirmPassword === "string" ? payload.confirmPassword : payload.confirmPassword || undefined,
    affiliateId: trimStr(payload.affiliateId),
    visitorId: trimStr(payload.visitorId),
  };

  const schema = buildSchema();

  try {
    const cleaned = schema.validateSync(normalized, { abortEarly: false, stripUnknown: true });
    return { errors: [], cleaned };
  } catch (e) {
    const errors = Array.isArray(e.errors) ? e.errors : ["Invalid arguments"];
    // Still return partial cleaned data from normalization
    return { errors, cleaned: normalized };
  }
}

Here, we "normalize" the inputs, which repeats most of the logic of how we validate the inputs. Mostly. This does have the added benefit of ensuring that the password fields could be undefined, which is not null. More fun, to my mind, is that the input form is clearly inconsistent about the naming of fields- is it telephone or phoneNumber? email or emailAddress?

I agree that this is cursed, less in the creeping dread sense, and more in the "WTF" sense.

[Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!