3
\$\begingroup\$

This application is the front end to a very basic database application. The front end assumes that the back end database would have fields as per the html form.

some concerns on db2form.js I have are:

  1. some very specific html doc references in the javascript - eg document.forms.searchform.elements.search.innerText = "Search";

  2. current_contact_idx global variable doesn't seem right.

As for the css file, that could probably be improved a lot.

Any feedback on this application would be very welcome.

The html page:

<!DOCTYPE html>
<html lang="en">
  <head>
  <meta charset="UTF-8">
  <title>Itel Office</title>
  <link rel="stylesheet" href="style.css">

<script src="db2form.js"></script>

  </head>
  <body>
    <nav>
      <a href="">Contacts</a>
      <a href="call_identifier_pretty.html" target="_blank">Call Log</a>
    </nav>
    <section>
      <h1>Contacts</h1>

  <p>Enter text below and click Search button to find a contact</p>
  <form name="searchform" action="/cgi-bin/database.exe" method="POST">
  <label for="rowid">ID: </label>
  <input id="rowid" type="text" name="rowid" value="" readonly disabled>
<br>
  <label for="name">Name: </label>
  <input id="name" type="text" name="name" value="">
<br>
<label for="company">Company: </label>
<input id="company" type="text" name="company" value="">
<br>

<label for="email">Email: </label>
<input id="email" type="email" name="email" value="">
<br>
<label for="ddi">Telephone: </label>
<input id="ddi" type="tel" name="ddi" value="">
<br>
<label for="mobile">Mobile: </label>
<input id="mobile" type="tel" name="mobile" value="">
<br>
<label for="switchboard">alt Telephone: </label>
<input id="switchboard" type="tel" name="switchboard" value="">
<br>
<label for="url">Web: </label>
<input id="url" type="text" name="url" value="">
<br>
<label for="address1">Address line 1: </label>
<input id="address1" type="text" name="address1" value="">
<br>
<label for="address2">Address line 2: </label>
<input id="address2" type="text" name="address2" value="">
<br>
<label for="address3">Address line 3: </label>
<input id="address3" type="text" name="address3" value="">
<br>
<label for="address4">Address line 4: </label>
<input id="address4" type="text" name="address4" value="">
<br>
<label for="postcode">Postcode: </label>
<input id="postcode" type="text" name="postcode" value="">
<br>
<label for="category">Category: </label>
<input id="category" type="text" name="category" value="">
<br>
<label for="notes">Notes: </label>
<textarea id="notes" name="notes"></textarea>
<br>

<div class="buttons">
<button name="search" type="button" onclick="process(document.forms.searchform.elements.search.innerText)">Search</button>

<button name="new" type="button" onclick="process('New')">New</button>

<button name="edit" type="button" onclick="process('Edit')" disabled>Edit</button>
<button name="save" type="button" onclick="process('Save')" disabled>Save</button>
<button name="delete" type="button" onclick="process('Delete')" disabled>Delete</button>

<button name="first" type="button" onclick="process('First')" disabled>First</button>
<button name="next" type="button" onclick="process('Next')" disabled>Next</button>
<button name="prior" type="button" onclick="process('Prior')"disabled>Prior</button>
<button name="last" type="button" onclick="process('Last')" disabled>Last</button>

</div>
</form>

  <div id="status">
  </div>

    </section>
  </body>
</html>

The css file, style.css:

body{
    background-color: #ffff00;
}

nav{
    box-sizing:border-box;
    background-color:#409fff;  /* blue we like */
    display: inline-block;
    width: 20%;
    min-width: 125px;
    margin-right:15px;
    height:100vh;
    overflow: auto;
}

nav a{
    display:block;
    line-height: 45px;
    height:45px;
    color: #FFFFFF;
    text-decoration: none;
    padding-left: 50px;
    margin:10px 0 10px 5px;
}

section{
    display: inline-block;
    width:70%;
    height:100vh;
    overflow: auto;
}

h1{
     color: #409fff;
     padding: 2px;
     margin: 0;
}

form {
    display: grid;
     grid-template-columns: 150px 1fr;
     border: 0;
}

label {
    grid-column: 1 / 2;
    margin: 0;
    padding:0;
     border: 0;
}

input{
    grid-column: 2 / 3;
    margin: 0;
    padding:0;
     border: 0;
     border-radius: 5px;
}

/*input:focus{
     background-color: #fcfab1;
}
*/
textarea{
     border-radius: 5px;
     height: 20px;
}


.buttons{
     display: grid;

     grid-column: 2 / 3;
     grid-gap: 10px;
     grid-template-columns: 1fr 1fr 1fr 1fr 1fr 1fr 1fr 1fr 1fr;
}

The javascript file, db2form.js:

let current_contact_idx = -1;
let records = null;

function search_mode() {
    // now change button to say Search
    document.forms.searchform.elements.search.innerText = "Search";
    document.forms.searchform.elements.new.disabled = false;

    document.forms.searchform.elements.edit.disabled = true;
    document.forms.searchform.elements.save.disabled = true;
    document.forms.searchform.elements.delete.disabled = true;

    document.forms.searchform.elements.first.disabled = true;
    document.forms.searchform.elements.next.disabled = true;
    document.forms.searchform.elements.prior.disabled = true;
    document.forms.searchform.elements.last.disabled = true;
}

function found_mode() {
    // now change button to say Cancel
    document.forms.searchform.elements.search.innerText = "Cancel";
    document.forms.searchform.elements.new.disabled = false;

    document.forms.searchform.elements.edit.disabled = false;
    document.forms.searchform.elements.save.disabled = true;
    document.forms.searchform.elements.delete.disabled = false;

    document.forms.searchform.elements.first.disabled = false;
    document.forms.searchform.elements.next.disabled = false;
    document.forms.searchform.elements.prior.disabled = false;
    document.forms.searchform.elements.last.disabled = false;
}

function new_edit_mode() {
    // now change button to say Cancel
    document.forms.searchform.elements.search.innerText = "Cancel";
    document.forms.searchform.elements.new.disabled = true;

    document.forms.searchform.elements.edit.disabled = true;
    document.forms.searchform.elements.save.disabled = false;
    document.forms.searchform.elements.delete.disabled = true;

    document.forms.searchform.elements.first.disabled = true;
    document.forms.searchform.elements.next.disabled = true;
    document.forms.searchform.elements.prior.disabled = true;
    document.forms.searchform.elements.last.disabled = true;
}

function server_response_callback_search(ajax) {

    let form_elements = document.forms.searchform.elements;

     if(ajax.responseText.length == 0) {
        cancel_step(form_elements);
        document.getElementById('status').innerHTML = "No record found for your search." 
        return;
    }

     console.log("server_response_callback_search response type: " + ajax.getResponseHeader('content-type'));

    records = JSON.parse(ajax.responseText);
    if (records.contacts.length > 0) {
          current_contact_idx = 0;
        populate_field(records.contacts[current_contact_idx]);

        found_mode();
    } else {
        current_contact_idx = -1;  // reset to no record found
          search_mode();  // stay in search mode
     }

    // display message
    if (current_contact_idx == -1) {
        document.getElementById('status').innerHTML = "No record found which matches the criteria";
    } else {
        document.getElementById('status').innerHTML = "Displaying record " + (current_contact_idx + 1).toString() + " of " + records.contacts.length;
    }
}

function server_response_callback_update(ajax, rowid) {

     console.log("server_response_callback_update response type: " + ajax.getResponseHeader('content-type'));
    let form_elements = document.forms.searchform.elements;

    search_mode();

    // empty all input and textarea fields
    for (let element of form_elements) {
        if(element.type != 'hidden') {
            element.value = "";
        }
    }

    document.getElementById('status').innerHTML = ajax.responseText;;
}

function server_response_callback_insert(ajax) {

     console.log("server_response_callback_insert response type: " + ajax.getResponseHeader('content-type'));
    let form_elements = document.forms.searchform.elements;

    search_mode();

    // empty all input and textarea fields
    for (let element of form_elements) {
        if(element.type != 'hidden') {
            element.value = "";
        }
    }

    document.getElementById('status').innerHTML = ajax.responseText;
}


// We need to display what it is that database.exe returns for these cases

function server_response_callback_delete(ajax, rowid) {
     console.log("server_response_callback_delete response type: " + ajax.getResponseHeader('content-type'));
    let form_elements = document.forms.searchform.elements;

    search_mode();

    // empty all input and textarea fields
    for (let element of form_elements) {
        if(element.type != 'hidden') {
            element.value = "";
        }
    }

    document.getElementById('status').innerHTML = ajax.responseText;
}

function populate_field(element) {
    let formelements = document.forms.searchform.elements;

    // formelements is an array
     for (let i = 0; i < formelements.length; i++) {
          if (formelements[i].name in element) {
            formelements[i].value = element[formelements[i].name];
          } else {
                formelements[i].value = "";
          }
     }
     document.getElementById('status').innerHTML = "Displaying record " + (current_contact_idx + 1).toString() + " of " + records.contacts.length;
}

function edit_step() {
     new_edit_mode();
}

function cancel_step(form_elements) {
     search_mode();

    // empty all input and textarea fields
    for (let element of form_elements) {
        if(element.type != 'hidden') {
            element.value = "";
        }
    }
    document.getElementById('status').innerHTML = "";
}

function new_step(form_elements) {
    new_edit_mode();

    // empty all input and textarea fields
    for (let element of form_elements) {
        if(element.type != 'hidden') {
            element.value = "";
        }
    }
    document.getElementById('status').innerHTML = "Enter data for new contact, then click Save button to save to database";
}

function extract_form_values(form_elements) {
    let query = "";
    let first = "yes";
    for (let element of form_elements) {
        if(["text", "textarea", "tel", "email"].includes(element.type)) {
            if(first == "no") {
                query += "&";
            }
            first = "no";
            query += element.name;
            query += "=";
            query += element.value;
        }
    }
    return query;
}

function save_step(form_elements) {
    let request_payload = extract_form_values(form_elements);
    if(request_payload.length == 0) {
        //alert("You need to enter some data to save to database");
        document.getElementById('status').innerHTML = "You need to enter some data to save to database";
        return;
    }

    // we determine whether to UPDATE or INSERT based on presence of rowid.
     // if a rowid assume updating an existing contact, otherwise a new contact
    if (document.forms.searchform.elements.rowid.value == "") {
       // go down INSERT route
        // remove rowid= from payload
          let pos = request_payload.indexOf("rowid=&");
          if (pos != -1) {
            // remove string
                request_payload = request_payload.replace("rowid=&", "");
          }
        request_payload += "&operation=INSERT";
        console.log("sending query to database server: " + request_payload);

        // setup ajax callback to handle response
        ajax_post("/cgi-bin/database.exe", request_payload, server_response_callback_insert);

     } else {
        let rowid = parseInt(document.forms.searchform.elements.rowid.value, 10);

        request_payload += "&operation=UPDATE";
        console.log("sending query to database server: " + request_payload);

        // setup ajax callback to handle response
        ajax_post("/cgi-bin/database.exe", request_payload, server_response_callback_update, rowid);
     }
}

function has_values(form_elements) {

     for (let element of form_elements) {
        if(["text", "textarea", "tel", "email"].includes(element.type) && element.name != "rowid" && element.value != "") {
            return true;
          }
     }
     return false;
}

function insert_step(form_elements) {

     // check user actually entered some data in fields
     if(!has_values(form_elements)) {
        console.log("attempting to insert but no values populated");
          document.getElementById('status').innerHTML = "Enter contact details to add a new contact";
          return;
     }

    let request_payload = extract_form_values(form_elements);
    if(request_payload.length == 0) {
        document.getElementById('status').innerHTML = "You need to enter some update a contact";
        return;
    }

    request_payload += "&operation=INSERT";
    console.log("sending query to database server: " + request_payload);

    // setup ajax callback to handle response
    ajax_post("/cgi-bin/database.exe", request_payload, server_response_callback_insert);
}

function search_step(form_elements) {
    let query = extract_form_values(form_elements);
    query += query.length == 0 ? "operation=SELECT" : "&operation=SELECT";
    console.log("sending query to database server: " + query);

    // setup ajax callback to handle response
    ajax_post("/cgi-bin/database.exe", query, server_response_callback_search);
}

function ajax_post(url, request, callback, arg) {
    // setup ajax callback to handle response
    var xhttp = new XMLHttpRequest();
    xhttp.onreadystatechange = function() {
        if (this.readyState == 4 && this.status == 200) {
            callback(this, arg);
        }
    };

    xhttp.open("POST", url, true);
    xhttp.setRequestHeader("Content-type", "application/x-www-form-urlencoded");
    xhttp.send(request);
}

function delete_step(form_elements) {

     if(form_elements.rowid.value == "") {
          const delete_msg = "Form not in correct state to delete a contact";
          document.getElementById('status').innerHTML = delete_msg;
        alert(delete_msg);
          return;
     }

     let rowid = parseInt(form_elements.rowid.value, 10);

     // DELETE FROM table_name WHERE condition;
     let request = `rowid=${rowid}&operation=DELETE`;
    console.log("sending request to database server: " + request);

     let confirmation = confirm("Click Ok if you are absolutely sure you want to delete this contact from the database");
    if (confirmation) {
        // setup ajax callback to handle response
        ajax_post("/cgi-bin/database.exe", request, server_response_callback_delete, rowid);
    }
}

function process(buttontext) {
    console.log(`buttontext=${buttontext}`);

    let form_elements = document.forms.searchform.elements;

     if (buttontext == "New") {
        new_step(form_elements);
    }else if (buttontext == "Edit") {
        edit_step();
    } else if (buttontext == "Save") {
        save_step(form_elements);
    } else if (buttontext == "Search") {
        search_step(form_elements);
    } else if (buttontext == "Cancel") {
        cancel_step(form_elements);
     } else if (buttontext == "Delete") {
          delete_step(form_elements);
     } else if (buttontext == "First") {
          if (records.contacts.length != 0) {
                current_contact_idx = 0;
            populate_field(records.contacts[current_contact_idx]);
          }
     } else if (buttontext == "Next") {
          if (records.contacts.length > (current_contact_idx + 1)) {
                populate_field(records.contacts[++current_contact_idx]);
          } else {
                document.getElementById('status').innerHTML = "You are on the last record";
          }
     } else if (buttontext == "Prior") {
          if (current_contact_idx > 0) {
                populate_field(records.contacts[--current_contact_idx]);
          } else {
                document.getElementById('status').innerHTML = "You are on the first record";
          }
     } else if (buttontext == "Last") {
          if (records.contacts.length != 0) {
                current_contact_idx = records.contacts.length - 1;
            populate_field(records.contacts[current_contact_idx]);
          }
    } else {
        document.getElementById('status').innerHTML = "something has gone wrong - button text incorrectly set";
    }
}

// user can press Enter key to invoke search, Esc key to cancel (go back to ready to search mode)
document.onkeydown = function(evt) {
    evt = evt || window.event;
    var isEscape = false;
     var isEnter = false;
    if ("key" in evt) {
        isEscape = (evt.key === "Escape" || evt.key === "Esc");
        isEnter = (evt.key === "Enter");
    } else {
        isEscape = (evt.keyCode === 27);
        isEnter = (evt.keyCode === 13);
    }
    if (isEscape) {
        // only handle Escape if Cancel button enabled
        if(document.forms.searchform.elements.search.innerText == "Cancel") {
            process("Cancel");
        }
    } else if (isEnter) {
        // only handle Enter if Search button enabled
        if(document.forms.searchform.elements.search.innerText == "Search") {
            process("Search");
        }
    }
};
\$\endgroup\$
  • \$\begingroup\$ What is /cgi-bin/database.exe? The hacker in me gets all warm and fuzzy when it sees an executable exposed to the internet. \$\endgroup\$ – konijn Apr 27 at 11:36
  • \$\begingroup\$ @konijn You think this is less secure than if for example action was: /cgi-bin/database.pl or how about /cgi-bin/database.cgi? \$\endgroup\$ – arcomber Apr 27 at 14:58
  • \$\begingroup\$ Yup, now I know you run Windows. owasp-aasvs.readthedocs.io/en/latest/requirement-8.1.html \$\endgroup\$ – konijn Apr 27 at 15:14
  • \$\begingroup\$ @konijn Good point, noted \$\endgroup\$ – arcomber Apr 27 at 15:20
2
\$\begingroup\$

On the Javascript side of things:

Don't use inline handlers, they have way too many problems to be worth using. Instead, attach listeners with Javascript and addEventListener instead.

Since on every button click including Search, you want to pass the text content of the button to process, you can do that concisely by examining the textContent of the clicked button inside the handler.

It's generally preferable to select elements with querySelector (which accepts concise, flexible CSS strings) rather than going through document.forms:

document.querySelector('.buttons').addEventListener('click', ({ target }) => {
  if (!target.matches('button')) return;
  process(target.textContent);
});

Using the above code will allow you to remove all inline handlers from the .buttons > button elements, including the onclick="process(document.forms.searchform.elements.search.innerText)".

const process = console.log;
document.querySelector('.buttons').addEventListener('click', ({ target }) => {
  if (!target.matches('button')) return;
  process(target.textContent);
});
<div class="buttons">
  <button name="search" type="button">Search</button>
  <button name="new" type="button">New</button>
  <button name="edit" type="button" disabled>Edit</button>
  <button name="save" type="button" disabled>Save</button>
  <button name="delete" type="button" disabled>Delete</button>
  <button name="first" type="button" disabled>First</button>
  <button name="next" type="button" disabled>Next</button>
  <button name="prior" type="button" disabled>Prior</button>
  <button name="last" type="button" disabled>Last</button>
</div>

Best to use textContent, the standard way to extract text from an element, not innerText, a peculiar property from Internet Explorer that has pretty strange behavior. innerText is almost never what you want.

Rather than selecting the buttons over and over again in search_mode, found_mode, new_edit_mode, consider selecting them once, and construct an object indexed by element type instead:

const buttons = {};
for (const button of document.querySelectorAll('.buttons > button')) {
  buttons[button.textContent.toLowerCase()] = button;
}

function enableDisableButtons(newVal) {
  for (const button of buttons) {
    button.disabled = newVal;
  }
}
function search_mode() {
  buttons.search.textContent = 'Search';
  enableDisableButtons(true);
  buttons.new.disabled = false;
}

function found_mode() {
  buttons.search.textContent = 'Cancel';
  enableDisableButtons(false);
  buttons.save.disabled = true;
}

function new_edit_mode() {
  buttons.search.textContent = 'Cancel';
  enableDisableButtons(true);
  buttons.save.disabled = false;
}

You can also save a reference to the status element instead of re-selecting it frequently.

const status = document.querySelector('#status');
// ...
status.innerHTML = "Displaying record " + (current_contact_idx + 1).toString() + " of " + records.contacts.length;

The above code also points to another issue - unless you're deliberately inserting HTML markup, you should set text content of elements by assigning to textContent, not innerHTML. Using innerHTML can result in arbitrary code execution if the code is untrustworthy, in addition to being slower than textContent and more confusing for script readers. So, for the above, you'd want to instead do

status.textContent = "Displaying record " + (current_contact_idx + 1).toString() + " of " + records.contacts.length;

In your process function, rather than a whole bunch of if/else checks on the argument, you could consider making an object indexed by the button text instead, whose values are the function you'd want to run when that button needs to be processed. In the handler, just look up the function on the object and run it:

const actionsByButtonText = {
  New: new_step,
  Edit: edit_step,
  Save: save_step,
  // ...
};
function process(buttontext) {
  console.log(`buttontext=${buttontext}`);
  const fn = actionsByButtonText[buttontext];
  if (fn) fn();
  else status.textContent = "something has gone wrong - button text incorrectly set";
}

(No need to pass form_elements to those functions - they can iterate through the buttons object above, it doesn't make much sense as an argument, since it never changes)

It looks like you're using let by default when declaring variables. Best to always use const - don't use let unless you have to reassign, and never use var (like in your ajax_post). Using const indicates to later readers of the script, including you, that the variable name will never be reassigned, which results in less cognitive overhead than permitting reassignment with let.

In Javascript, variables are almost always named using camelCase, which you might want to consider if you want to conform.

The script is a bit long - originally, 371 lines. Once you have a script with more than 3-4 functions, I'd strongly consider organizing it using modules instead. Having separate modules which each do their own thing is more maintainable than having one big file. Modules are also useful because the dependencies between them are explicit, rather than everything being global and being able to potentially reference everything else - that can make things a bit confusing when the code isn't trivial. Look into something like webpack.

You should also consider using proper indentation in the HTML, it'll make the structure more readable at a glance. Eg, this:

  </nav>
  <section>
    <h1>Contacts</h1>

<p>Enter text below and click Search button to find a contact</p>
<form name="searchform" action="/cgi-bin/database.exe" method="POST">
<label for="rowid">ID: </label>
<input id="rowid" type="text" name="rowid" value="" readonly disabled>

should probably be

</nav>
<section>
  <h1>Contacts</h1>
  <p>Enter text below and click Search button to find a contact</p>
  <form name="searchform" action="/cgi-bin/database.exe" method="POST">
    <label for="rowid">ID: </label>
    <input id="rowid" type="text" name="rowid" value="" readonly disabled>
    ...

There are other improvements that can be made as well, but this should be a good start.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.