CUTCODEDOWN
Minimalist Semantic Markup

Welcome Guest
Please Login or Register

If you have registered but not recieved your activation e-mail in a reasonable amount of time, or have issues with using the registration form, please use our Contact Form for assistance. Include both your username and the e-mail you tried to register with.

Author Topic: This looks wrong - multiple forms?  (Read 92 times)

GrumpyYoungMan

  • Hero Member
  • *****
  • Posts: 599
  • Karma: +6/-0
    • GrumpyYoungMan
This looks wrong - multiple forms?
« on: 2 Nov 2021, 06:01:57 am »
Code: [Select]
while($r=$records->fetch()){
    $date = new DateTime($r['m_expiry']);
    echo '<tr>
    <td><div>', $r['v_vrm'],'</div></td>
    <td>', $date->format('F jS Y'),'</td>
    <td class="resultOptions">
        <form>
            <input type="hidden" name="mot" value="view">
            <button type="submit" name="id"  value="', $r['m_id'], '">view</button>
        </form>
       
        <form>
            <input type="hidden" name="mot" value="edit">
            <button type="submit" name="id"  value="', $r['m_id'], '">edit</button>
        </form>
       
        <form>
            <input type="hidden" name="mot" value="delete">
            <button type="submit" name="id"  value="', $r['m_id'], '">delete</button>
        </form>
       
</td>
</tr>';
 
}

I am trying to use the form button for the switch for View, Edit, Delete although it seems to work it just looks and feels wrong?

Comments/Suggestions? Please?
Trying to learn a new trick to prove old dogs can learn new ones...

Total Novice have-a go Amateur Programmer - not sure that is the right thing to say... but trying to learn...

benanamen

  • Full Member
  • ***
  • Posts: 158
  • Karma: +14/-0
Re: This looks wrong - multiple forms?
« Reply #1 on: 2 Nov 2021, 02:13:25 pm »
1. Get the date in the format you want it from the DB. No need for code gymnastics.
2. Use a GET hyperlink for the buttons instead of a form.
Code: [Select]
<td><a href="update.php?id=616"><button>Update</button></a></td>
To save time, let's just assume I am never wrong.

GrumpyYoungMan

  • Hero Member
  • *****
  • Posts: 599
  • Karma: +6/-0
    • GrumpyYoungMan
Re: This looks wrong - multiple forms?
« Reply #2 on: 2 Nov 2021, 03:11:01 pm »
1. Get the date in the format you want it from the DB. No need for code gymnastics.
2. Use a GET hyperlink for the buttons instead of a form.
Code: [Select]
<td><a href="update.php?id=616"><button>Update</button></a></td>

I was told using GET for sensitive options was insecure ? Using POST was better and more secure?
Trying to learn a new trick to prove old dogs can learn new ones...

Total Novice have-a go Amateur Programmer - not sure that is the right thing to say... but trying to learn...

benanamen

  • Full Member
  • ***
  • Posts: 158
  • Karma: +14/-0
Re: This looks wrong - multiple forms?
« Reply #3 on: 2 Nov 2021, 03:16:23 pm »
Quote
I was told using GET for sensitive options was insecure ? Using POST was better and more secure?

Neither one is more secure than the other. Both are "user supplied data" and must be validated.
To save time, let's just assume I am never wrong.

GrumpyYoungMan

  • Hero Member
  • *****
  • Posts: 599
  • Karma: +6/-0
    • GrumpyYoungMan
Re: This looks wrong - multiple forms?
« Reply #4 on: 2 Nov 2021, 04:16:46 pm »
So, is it down to my preference? Which one would be the preferred option?

I can’t find any info on the SQL date conversion?

Thanks for your input!
Trying to learn a new trick to prove old dogs can learn new ones...

Total Novice have-a go Amateur Programmer - not sure that is the right thing to say... but trying to learn...

Jason Knight

  • Administrator
  • Hero Member
  • *****
  • Posts: 775
  • Karma: +140/-1
    • CutCodeDown -- Minimalist Semantic Markup
Re: This looks wrong - multiple forms?
« Reply #5 on: 2 Nov 2021, 04:36:32 pm »
What makes it "Wrong" is those forms lack ACTION. In which case it's probably not a form, or is going full scripttard with zero graceful degradation.

A button inside an anchor being even worse since it's invalid markup AND pointless gibberish. BOTH perform actions and are therefor incompatible with each-other.

I don't know what any of that code is supposed to be trying to do, as not one line of it makes a lick of sense. USUALLY if you have a form wrapped that way it's so you can perform actions via POST instead of GET, since anchors can only "get". Using POST via a form helps prevent URI injections from doing things like deleting or editing from a link.

To that end -- and this is me guessing WILDLY.

Code: [Select]
while ($row = $records->fetch()){
$date = new DateTime($row['m_expiry']);
echo '
<tr>
<th scope="row">', htmlspecialchars($row['v_vrm']),'</th>
<td>', $date->format('F jS Y'),'</td>
<td>
<form action="whatever.php" method="post">
<input type="hidden" name="id" value="', htmlspecialchars($row['m_id']), '">
<button name="action" value="view">View</button>
<button name="action" value="edit">Edit</button>
<button name="action" value="delete">Delete</button>
<form>
</td>
</tr>';
}

Most people don't realize you can do that with buttons. Only the clicked button has its values sent server-side. Also, the default type for button is "submit", you don't need to explicitly say that. Also one of the few times I'd skip fieldset, none of those are user editable values.

It also helps to use variable names that aren't cryptic junk, and even if you think the data is trustworthy, specialchars it anyways!
I'll fix every flaw, I'll break every law, I'll tear up the rulebook if that's what it takes. You will see, I will crush this cold machine.

benanamen

  • Full Member
  • ***
  • Posts: 158
  • Karma: +14/-0
Re: This looks wrong - multiple forms?
« Reply #6 on: 2 Nov 2021, 04:51:45 pm »
A button inside an anchor being even worse since it's invalid markup AND pointless gibberish.

That's what I get for quickly grabbing something off a random site.

Quote
Using POST via a form helps prevent URI injections from doing things like deleting or editing from a link.

Only for the noobest of noobs. It is quite trivial to send a modified request to a server whether it be POST or GET, the browser Dev Tools being one of the most readily available ways.
« Last Edit: 2 Nov 2021, 04:54:48 pm by benanamen »
To save time, let's just assume I am never wrong.

Jason Knight

  • Administrator
  • Hero Member
  • *****
  • Posts: 775
  • Karma: +140/-1
    • CutCodeDown -- Minimalist Semantic Markup
Re: This looks wrong - multiple forms?
« Reply #7 on: 2 Nov 2021, 04:57:11 pm »
I was told using GET for sensitive options was insecure ? Using POST was better and more secure?
100% accurate. Because if for example your form submits to "whatever.php" there is nothing stopping someone from doing "whatever.php?id=1&mot=delete"

This is a constant worry on forum software, any action that could be used maliciously must wrap in a form for POST if you let users post anchors. Why? Because if I made a link in a post that contained the above, if YOU clicked on it? It would run with YOUR permissions, not mine.

Any secure operations -- edit, delete, etc, etc -- should be handled via POST so as to stop URI injection.
I'll fix every flaw, I'll break every law, I'll tear up the rulebook if that's what it takes. You will see, I will crush this cold machine.

Jason Knight

  • Administrator
  • Hero Member
  • *****
  • Posts: 775
  • Karma: +140/-1
    • CutCodeDown -- Minimalist Semantic Markup
Re: This looks wrong - multiple forms?
« Reply #8 on: 2 Nov 2021, 05:00:32 pm »
Only for the noobest of noobs. It is quite trivial to send a modified request to a server whether it be POST or GET, the browser Dev Tools being one of the most readily available ways.
Except that doing so only works using YOUR permissions (if any). IF you have someplace users can post their own links, and they sucker a moderator or admin or anyone else with edit rights into clicking on it? Then it runs with that admin's permissions!

For example if you had a "deleteUser.php" that checks that the logged in user is admin, doing "deleteUser.php?id=1" trying to delete the admin wouldn't work. But if you created a post on a forums that sent that exact same link, it would run with the permissions of whatever user clicks on it.

It's one of the most classic forum / CMS attacks. Generating your own server request isn't going to do dick if you don't have the permissions/cookies/tokens to perform that action.

Suckering someone with permissions into clicking on a link is a lot easier and gives you all of their permissions... and you can't put post data into a URI.
« Last Edit: 2 Nov 2021, 05:06:53 pm by Jason Knight »
I'll fix every flaw, I'll break every law, I'll tear up the rulebook if that's what it takes. You will see, I will crush this cold machine.

benanamen

  • Full Member
  • ***
  • Posts: 158
  • Karma: +14/-0
Re: This looks wrong - multiple forms?
« Reply #9 on: 2 Nov 2021, 05:23:06 pm »
I generally do not disagree with what you have stated but if all it takes for an unauthorized delete is to add deleteUser.php?id=1 then your app is poorly coded.

Your own forum right here uses a GET request to add and delete posts. I cant just add deleteUser.php?id=1 and delete your posts or post a link that anyone else can click to do it either. And neither can I modify the URL to make you delete your own post. As you said..
Quote
Generating your own server request isn't going to do dick if you don't have the permissions/cookies/tokens to perform that action.

For this forum I would need your current session id to make a valid link to fake you out and make you delete one of your own posts. So, properly coded, GET or POST doesn't matter for security. If I have your session ID to make a fake link I could just as easily send a POST request and delete it myself if the app was expecting a POST request to delete.
« Last Edit: 2 Nov 2021, 05:40:54 pm by benanamen »
To save time, let's just assume I am never wrong.

GrumpyYoungMan

  • Hero Member
  • *****
  • Posts: 599
  • Karma: +6/-0
    • GrumpyYoungMan
Re: This looks wrong - multiple forms?
« Reply #10 on: 3 Nov 2021, 02:41:59 am »
Thank you for your advice and feedback I’ll revisit the form when I get to the office!
Trying to learn a new trick to prove old dogs can learn new ones...

Total Novice have-a go Amateur Programmer - not sure that is the right thing to say... but trying to learn...

GrumpyYoungMan

  • Hero Member
  • *****
  • Posts: 599
  • Karma: +6/-0
    • GrumpyYoungMan
Re: This looks wrong - multiple forms?
« Reply #11 on: 8 Nov 2021, 03:48:00 am »
It may not be pretty, but it seems to work, so thank you for your help and advice!

Code: [Select]
while ($r = $records->fetch()) {
        $date = new DateTime($r['m_expiry']);
        $diff = date_diff(date_create(date("Y-m-d")), date_create($r['m_expiry']));
        echo '<tr>
    <td><div>', $r['v_vrm'], '</div></td>
    <td>', $date->format('F jS Y'), '</td>
    <td><span class="', (!empty($diff->invert) ? 'expired' : ($diff->days <= 30 ? 'expiring' : 'valid')), '">', (!empty($diff->invert) ? 'expired' : ($diff->days <= 30 ? 'expiring' : 'valid')), '</span></td>
    <td class="resultOptions">
        <form method="post" action="?mot">       
            <input type="hidden" name="id" value="', $r['m_id'], '">
            <button type="submit" name="mot"  value="view">view</button>       
            <button type="submit" name="mot"  value="update">update</button>       
            <button type="submit" name="mot"  value="delete">delete</button>
        </form>       
</td>
</tr>';
    }
Trying to learn a new trick to prove old dogs can learn new ones...

Total Novice have-a go Amateur Programmer - not sure that is the right thing to say... but trying to learn...

GrumpyYoungMan

  • Hero Member
  • *****
  • Posts: 599
  • Karma: +6/-0
    • GrumpyYoungMan
Re: This looks wrong - multiple forms?
« Reply #12 on: 8 Nov 2021, 03:51:06 am »
I generally do not disagree with what you have stated but if all it takes for an unauthorized delete is to add deleteUser.php?id=1 then your app is poorly coded.

Your own forum right here uses a GET request to add and delete posts. I cant just add deleteUser.php?id=1 and delete your posts or post a link that anyone else can click to do it either. And neither can I modify the URL to make you delete your own post. As you said..
Quote
Generating your own server request isn't going to do dick if you don't have the permissions/cookies/tokens to perform that action.

For this forum I would need your current session id to make a valid link to fake you out and make you delete one of your own posts. So, properly coded, GET or POST doesn't matter for security. If I have your session ID to make a fake link I could just as easily send a POST request and delete it myself if the app was expecting a POST request to delete.
[off-topic]So, is it always best to ask for the user password for any sensitive action, ie delete record, etc?
Trying to learn a new trick to prove old dogs can learn new ones...

Total Novice have-a go Amateur Programmer - not sure that is the right thing to say... but trying to learn...

benanamen

  • Full Member
  • ***
  • Posts: 158
  • Karma: +14/-0
Re: This looks wrong - multiple forms?
« Reply #13 on: 8 Nov 2021, 11:31:44 am »

[off-topic]So, is it always best to ask for the user password for any sensitive action, ie delete record, etc?

No. Just make sure you use HTTPS and that the app does not have any XSS vulnerabilities. Of course you will also want solid authentication built into the app. Do test POSTS/GETS using a third party app such as Postman. It wouldn't hurt to read up on Session Hijacking so you understand how it can be done.
To save time, let's just assume I am never wrong.

 

SMF spam blocked by CleanTalk

Advertisement